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.

Sign up for GitHub

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

Jump to bottom

oci: Use controller-runtime pkg/log specifically #646

Merged
merged 2 commits into from
Sep 7, 2023
Merged

oci: Use controller-runtime pkg/log specifically #646

merged 2 commits into from
Sep 7, 2023

Conversation

darkowlzz
Copy link
Contributor

@darkowlzz darkowlzz commented Sep 6, 2023 •
edited

This helps avoid importing the controller-runtime pkg/client/config package which has a flag initialization for "kubeconfig". This results in all the users of the oci package to also have the flag initialized in their applications. The root level controller-runtime package has type aliases which we use a lot for ease of use, but the package also imports the client config package.
The usage of controller-runtime in oci package is just for logging. Importing the pkg/log package specifically helps avoid importing the client config which sets the flag.

Fixes #645

Verified by building kyverno locally and testing.

GCP integration test run: https://github.com/fluxcd/pkg/actions/runs/6098520241/job/16548522220

@darkowlzz
oci: Use controller-runtime pkg/log explicitly
f01d884 
This helps avoid importing the controller-runtime pkg/client/config
package which has a flag initialization for "kubeconfig". This results
in all the users of the oci package to also have the flag initialized in
their applications.
The usage of controller-runtime in oci package is just for logging.
Importing the pkg/log package specifically helps avoid importing the
client config which sets the flag.

Signed-off-by: Sunny <darkowlzz@protonmail.com>
@darkowlzz darkowlzz added enhancement New feature or request dependencies Pull requests that update a dependency area/oci OCI related issues and pull requests labels Sep 6, 2023
makkes
makkes approved these changes Sep 6, 2023
View reviewed changes
Copy link
Member

@makkes makkes left a comment

Choose a reason for hiding this comment

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

Perfect, thanks. I was able to verify that it works as expected when imported.

@makkes
Copy link
Member

makkes commented Sep 6, 2023

@darkowlzz we should add a test case that prevents a regression here.

@darkowlzz
Copy link
Contributor Author

darkowlzz commented Sep 6, 2023 •
edited

we should add a test case that prevents a regression here.

@makkes we can add a test that imports this package and checks for flags. But this is a very general problem for all the packages that use controller-runtime. Such a test will have to be added to all the other packages too to make sure they never import the root level controller-runtime package accidentally. I'm not sure how feasible that is. Once the upstream removes the flag initialization, all these work would become unnecessary.
But this seems to be a good problem to solve for an independent tool that go packages can import in their tests and run to make sure that they don't inject any unintended flags.

@makkes
Copy link
Member

makkes commented Sep 6, 2023 •
edited

But this is a very general problem for all the packages that use controller-runtime

We can start small by adding a test for this one. No need to make this just another huge project.

@vishal-chdhry vishal-chdhry mentioned this pull request Sep 6, 2023
Merged
9 tasks
@darkowlzz
oci/auth: Add test to check for non-test flags
e6669d8 
Add a black box test to import the auth package as a consumer of the
package and make sure that no flags are injected. Being in a test, it
ignores all the default test flags with "test." prefix.

Signed-off-by: Sunny <darkowlzz@protonmail.com>
@darkowlzz
Copy link
Contributor Author

I've added a small test under oci/auth as a black box test to import the login package which imports all the other packages. Being in test, the flag contains all the test flags. The test checks for existence of any non-test flag.

@darkowlzz darkowlzz changed the title oci: Use controller-runtime pkg/log explicitly oci: Use controller-runtime pkg/log specifically Sep 6, 2023
@realshuting
Copy link

Is there an ETA when this fix will be merged? We (Kyverno) encountered this issue while importing some of the fluxcd packages, would love to get this in!

@stefanprodan stefanprodan merged commit eedb1a0 into main Sep 7, 2023
13 checks passed
@stefanprodan stefanprodan deleted the oci-cr-logger branch September 7, 2023 08:06
@stefanprodan
Copy link
Member

@realshuting you can now use github.com/fluxcd/pkg/oci v0.31.1

@realshuting
Copy link

Thank you @stefanprodan!

@makkes
Copy link
Member

makkes commented Sep 7, 2023

Verified the test fails without the fix. Thanks @darkowlzz

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@makkes makkes makkes approved these changes

@stefanprodan stefanprodan stefanprodan approved these changes

Assignees
No one assigned
Labels
area/oci OCI related issues and pull requests dependencies Pull requests that update a dependency enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

bug: (pkg/oci/auth) Importing the auth package causes "kubeconfig flag redefined" error
4 participants
@darkowlzz @makkes @realshuting @stefanprodan