Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: toSelectorMatchFields #376

Merged

Conversation

devthejo
Copy link
Contributor

fix #348

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

devthejo commented Jun 2, 2023

I think the linter will be OK now :-)

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

require github.com/tidwall/gjson v1.14.4
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pushed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reverted, this lead to many git conflict

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it, fixed

@neil-hickey neil-hickey requested a review from rohitagg2020 June 5, 2023 21:34
@devthejo devthejo force-pushed the feat/match-selector-match-fields branch 3 times, most recently from 91d57a7 to ee2c04a Compare June 5, 2023 23:56
docs/secret-export.md Outdated Show resolved Hide resolved
@devthejo devthejo force-pushed the feat/match-selector-match-fields branch from f8e40df to 64fc04d Compare June 13, 2023 10:35
@revolunet
Copy link

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

Copy link
Member

@joaopapereira joaopapereira left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
go.mod Outdated
@@ -18,6 +18,8 @@ require (
sigs.k8s.io/controller-tools v0.11.3
)

require github.com/tidwall/gjson v1.14.4
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
@devthejo devthejo force-pushed the feat/match-selector-match-fields branch from e849c32 to 1e07fca Compare July 10, 2023 14:35
@devthejo
Copy link
Contributor Author

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

Copy link
Member

@joaopapereira joaopapereira left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
pkg/apis/secretgen2/v1alpha1/secret_export.go Outdated Show resolved Hide resolved
pkg/apis/secretgen2/v1alpha1/secret_export.go Outdated Show resolved Hide resolved
pkg/sharing/secret_exports.go Outdated Show resolved Hide resolved
pkg/sharing/secret_exports.go Outdated Show resolved Hide resolved
pkg/sharing/secret_exports.go Outdated Show resolved Hide resolved
pkg/sharing/secret_exports.go Outdated Show resolved Hide resolved
pkg/sharing/secret_exports.go Outdated Show resolved Hide resolved
@devthejo 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
Copy link
Contributor Author

devthejo commented Jul 10, 2023

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

@devthejo devthejo requested a review from joaopapereira July 10, 2023 23:00
Copy link
Member

@joaopapereira joaopapereira left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

config/package-bundle/config/crds.yml Outdated Show resolved Hide resolved
docs/secret-export.md Outdated Show resolved Hide resolved
@devthejo devthejo force-pushed the feat/match-selector-match-fields branch from 0381d30 to ac8c190 Compare July 11, 2023 21:52
@devthejo
Copy link
Contributor Author

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

@devthejo devthejo requested a review from joaopapereira July 11, 2023 21:59
Copy link
Member

@joaopapereira joaopapereira left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@joaopapereira
Copy link
Member

Can you please check the linting issue?

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

Can you please check the linting issue?

fixed

@devthejo
Copy link
Contributor Author

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

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

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
Copy link
Member

looks like that was a winner 😄

@joaopapereira joaopapereira merged commit f737917 into carvel-dev:develop Jul 13, 2023
@devthejo
Copy link
Contributor Author

Yeah !!!
Thanks for merging 😻

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

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

@joaopapereira
Copy link
Member

@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
Copy link

Thats very fine, thanks @joaopapereira

May the force be with you for #446

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
carvel-triage This issue has not yet been reviewed for validity
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Feature: match allow on namespace annotations
5 participants