feat: toSelectorMatchFields #376

devthejo · 2023-05-31T16:05:43Z

devthejo · 2023-06-02T21:16:29Z

I think the linter will be OK now :-)

neil-hickey · 2023-06-05T20:03:05Z

go.mod

@@ -18,6 +18,8 @@ require (
 	sigs.k8s.io/controller-tools v0.11.3
 )

+require github.com/tidwall/gjson v1.14.4


Not sure how this got moved outside the main require block, but if it's not too much trouble we should move it back in. I suspect it's an IDE thing. We can also sort this out post-merge.

it was the go cli, when I move the require to the upper block in vscodium and save it, all spaces are rewrited but if it's not a problem I can try to push that

reverted, this lead to many git conflict

Is there any particular reason for not using k8s.io/client-go/util/jsonpath instead of this library?
Also I would really like for us to have all the requires in the same grouping.

pkg/apis/secretgen2/v1alpha1/secret_export.go

neil-hickey · 2023-06-05T21:34:10Z

pkg/sharing/secret_exports.go

+			Name: nsName,
+		}
+		namespace := corev1.Namespace{}
+		err := matcher.SecretImportReconciler.client.Get(matcher.Ctx, query, &namespace)


I would prefer to see a client specifically for getting this namespace (which can be expanded in the future for other usecases) rather than re-using the import reconciler's client. Definitely feels like a borrowing of something we don't really need.

I didn't want to add more network connections (and I didn't knew the exact behavior of this, like persistent connection or something)
Have you a suggestion to the good way to create a new client ?

This does feel like an overreach inside the SecretImportReconciler to get a private attribute.
I think this struct should have an Interface that knows how to execute a Get, so something like:

type K8sReader interface { Get(ctx context.Context, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error }

Got it, fixed

docs/secret-export.md

revolunet · 2023-06-19T13:31:08Z

Hi, is there anything we can do to make this happen ?
That sounds like a great kubed replacement for our use-case :)
Thanks

joaopapereira

This is a good feature that is useful for a tool like Secretgen, but there is the cost associated with it of allowing people to copy secrets to places they should not be in. So let's make some changes to the naming of it to signal to the users that want to use it to be sure about it.
Also added some other comments about other parts of the code.

config/package-bundle/config/crds.yml

joaopapereira · 2023-07-07T22:20:50Z

go.mod

@@ -18,6 +18,8 @@ require (
 	sigs.k8s.io/controller-tools v0.11.3
 )

+require github.com/tidwall/gjson v1.14.4


Is there any particular reason for not using k8s.io/client-go/util/jsonpath instead of this library?
Also I would really like for us to have all the requires in the same grouping.

joaopapereira · 2023-07-07T22:32:43Z

pkg/sharing/secret_exports.go

+			Name: nsName,
+		}
+		namespace := corev1.Namespace{}
+		err := matcher.SecretImportReconciler.client.Get(matcher.Ctx, query, &namespace)


This does feel like an overreach inside the SecretImportReconciler to get a private attribute.
I think this struct should have an Interface that knows how to execute a Get, so something like:

type K8sReader interface { Get(ctx context.Context, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error }

pkg/sharing/secret_exports.go

devthejo · 2023-07-10T15:26:59Z

All should be good now, according to your feedbacks :-)

joaopapereira

Hey
Thanks for all the changes. It is looking much better now 😄

I have some questions/nitpicks that I would love for you to look into.
Also, adding some tests to ensure that we cover all the Operators we have created would be great. Do we have any tests to ensure that if you provide multiple selectors, the namespace matches all of them?

docs/secret-export.md

pkg/apis/secretgen2/v1alpha1/secret_export.go

pkg/sharing/secret_exports.go

devthejo · 2023-07-10T22:55:58Z

Thanks for your time and for the feedback :-)
I've done the refactos and added tests for each operator.
I think all has been fixed now

joaopapereira

Cool, I think we are almost there just have two asks:

I just reread the documentation and have some nitpicks there
Can you please squash your commits

After this, I think we are good to get this PR merged.

config/package-bundle/config/crds.yml

docs/secret-export.md

devthejo · 2023-07-11T21:56:54Z

Cool, I think we are almost there just have two asks:
1. I just reread the documentation and have some nitpicks there

2. Can you please squash your commits
After this, I think we are good to get this PR merged.

Thanks for the fixs on documentation, your corrections has been commited
Commits squashed :-)

I'm very glad to hear that,
many thanks for the collab

joaopapereira

LGTM

joaopapereira · 2023-07-13T16:33:28Z

Can you please check the linting issue?

devthejo · 2023-07-13T19:25:57Z

Can you please check the linting issue?

fixed

devthejo · 2023-07-13T19:33:47Z

I didn't see this message on first lint run, I check that now

Signed-off-by: devthejo <jo@surikat.pro>

devthejo · 2023-07-13T19:43:46Z

I fixed the last error message, but couldn't reproduce locally:
runnning golangci-lint run --out-format=github-actions -v, produced:

INFO [config_reader] Config search paths: [./ /lab/fabrique/sre/secretgen-controller /lab/fabrique/sre /lab/fabrique /lab / /home/jo] 
INFO [config_reader] Used config file .golangci.yml 
INFO [lintersdb] Active 3 linters: [goheader revive unused] 
INFO [loader] Go packages loading at mode 575 (compiled_files|files|name|types_sizes|deps|exports_file|imports) took 459.602192ms 
INFO [runner/filename_unadjuster] Pre-built 0 adjustments in 4.339922ms 
INFO [linters_context/goanalysis] analyzers took 1.263021127s with top 10 stages: the_only_name: 752.958032ms, goheader: 476.895239ms, unused: 21.007981ms, directives: 9.250207ms, isgenerated: 2.909668ms 
INFO [runner] Issues before processing: 64, after processing: 0 
INFO [runner] Processors filtering stat (out/in): skip_files: 64/64, exclude: 64/64, filename_unadjuster: 64/64, path_prettifier: 64/64, uniq_by_line: 64/64, cgo: 64/64, skip_dirs: 64/64, autogenerated_exclude: 64/64, nolint: 64/64, identifier_marker: 64/64, exclude-rules: 64/64, diff: 0/64 
INFO [runner] processing took 732.528196ms with stages: diff: 729.352011ms, identifier_marker: 1.328104ms, nolint: 1.234572ms, autogenerated_exclude: 272.056µs, path_prettifier: 270.508µs, skip_dirs: 44.28µs, uniq_by_line: 13.495µs, cgo: 5.872µs, filename_unadjuster: 3.65µs, max_same_issues: 947ns, source_code: 608ns, skip_files: 344ns, fixer: 333ns, max_per_file_from_linter: 215ns, severity-rules: 212ns, sort_results: 205ns, exclude: 200ns, path_shortener: 179ns, exclude-rules: 168ns, max_from_linter: 158ns, path_prefixer: 79ns 
INFO [runner] linters took 1.595293726s with stages: goanalysis_metalinter: 862.686857ms 
INFO File cache stats: 0 entries of total size 0B 
INFO Memory: 22 samples, avg is 150.7MB, max is 259.9MB 
INFO Execution took 2.064290429s

so I hope all will be good on next lint running on ci, but couldn't guarantee

joaopapereira · 2023-07-13T19:44:40Z

looks like that was a winner 😄

devthejo · 2023-07-13T19:55:47Z

Yeah !!!
Thanks for merging 😻

revolunet · 2023-09-04T08:55:26Z

Hello @joaopapereira, any idea when this could be released ? thanks !

joaopapereira · 2023-09-05T16:01:13Z

@revolunet Hey we should be able to release it in the next couple of weeks, we are trying to get #446 delivered before we do that. Is that ok or are you in a rush?

revolunet · 2023-09-06T08:32:54Z

Thats very fine, thanks @joaopapereira

May the force be with you for #446

devthejo force-pushed the feat/match-selector-match-fields branch from 2fc50f2 to d713cf1 Compare May 31, 2023 16:06

This was referenced May 31, 2023

feat: match ns annotations #337

Closed

Feature: match allow on namespace annotations #348

Closed

neil-hickey reviewed Jun 5, 2023

View reviewed changes

pkg/apis/secretgen2/v1alpha1/secret_export.go Show resolved Hide resolved

neil-hickey reviewed Jun 5, 2023

View reviewed changes

neil-hickey requested a review from rohitagg2020 June 5, 2023 21:34

devthejo force-pushed the feat/match-selector-match-fields branch 3 times, most recently from 91d57a7 to ee2c04a Compare June 5, 2023 23:56

rohitagg2020 reviewed Jun 13, 2023

View reviewed changes

docs/secret-export.md Outdated Show resolved Hide resolved

devthejo force-pushed the feat/match-selector-match-fields branch from f8e40df to 64fc04d Compare June 13, 2023 10:35

joaopapereira requested changes Jul 7, 2023

View reviewed changes

devthejo force-pushed the feat/match-selector-match-fields branch from e849c32 to 1e07fca Compare July 10, 2023 14:35

joaopapereira requested changes Jul 10, 2023

View reviewed changes

devthejo force-pushed the feat/match-selector-match-fields branch 3 times, most recently from 29fb29b to 76a2a55 Compare July 10, 2023 22:55

devthejo requested a review from joaopapereira July 10, 2023 23:00

joaopapereira requested changes Jul 11, 2023

View reviewed changes

config/package-bundle/config/crds.yml Outdated Show resolved Hide resolved

docs/secret-export.md Outdated Show resolved Hide resolved

devthejo force-pushed the feat/match-selector-match-fields branch from 0381d30 to ac8c190 Compare July 11, 2023 21:52

devthejo requested a review from joaopapereira July 11, 2023 21:59

joaopapereira approved these changes Jul 13, 2023

View reviewed changes

devthejo force-pushed the feat/match-selector-match-fields branch from ac8c190 to a379ddd Compare July 13, 2023 19:25

feat: dangerousToNamespacesSelector

c7b0284

Signed-off-by: devthejo <jo@surikat.pro>

devthejo force-pushed the feat/match-selector-match-fields branch from a379ddd to c7b0284 Compare July 13, 2023 19:41

joaopapereira merged commit f737917 into carvel-dev:develop Jul 13, 2023
5 checks passed

github-actions bot added the carvel-triage This issue has not yet been reviewed for validity label Jul 13, 2023

devthejo mentioned this pull request Jul 17, 2023

secrets management : etudier secretgen-controller SocialGouv/support#409

Closed

revolunet mentioned this pull request Jul 18, 2023

secretgen-controller SocialGouv/support#448

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: toSelectorMatchFields #376

feat: toSelectorMatchFields #376

devthejo commented May 31, 2023

devthejo commented Jun 2, 2023

neil-hickey Jun 5, 2023

devthejo Jun 5, 2023

devthejo Jun 5, 2023

devthejo Jun 5, 2023

joaopapereira Jul 7, 2023

neil-hickey Jun 5, 2023

devthejo Jun 5, 2023

joaopapereira Jul 7, 2023

devthejo Jul 10, 2023

revolunet commented Jun 19, 2023

joaopapereira left a comment

joaopapereira Jul 7, 2023

joaopapereira Jul 7, 2023

devthejo commented Jul 10, 2023

joaopapereira left a comment

devthejo commented Jul 10, 2023 •

edited

Loading

joaopapereira left a comment

devthejo commented Jul 11, 2023

joaopapereira left a comment

joaopapereira commented Jul 13, 2023

devthejo commented Jul 13, 2023

devthejo commented Jul 13, 2023

devthejo commented Jul 13, 2023

joaopapereira commented Jul 13, 2023

devthejo commented Jul 13, 2023

revolunet commented Sep 4, 2023

joaopapereira commented Sep 5, 2023

revolunet commented Sep 6, 2023

feat: toSelectorMatchFields #376

feat: toSelectorMatchFields #376

Conversation

devthejo commented May 31, 2023

devthejo commented Jun 2, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

revolunet commented Jun 19, 2023

joaopapereira left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devthejo commented Jul 10, 2023

joaopapereira left a comment

Choose a reason for hiding this comment

devthejo commented Jul 10, 2023 • edited Loading

joaopapereira left a comment

Choose a reason for hiding this comment

devthejo commented Jul 11, 2023

joaopapereira left a comment

Choose a reason for hiding this comment

joaopapereira commented Jul 13, 2023

devthejo commented Jul 13, 2023

devthejo commented Jul 13, 2023

devthejo commented Jul 13, 2023

joaopapereira commented Jul 13, 2023

devthejo commented Jul 13, 2023

revolunet commented Sep 4, 2023

joaopapereira commented Sep 5, 2023

revolunet commented Sep 6, 2023

devthejo commented Jul 10, 2023 •

edited

Loading