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

Add custom cache on KCP #70

Merged
merged 1 commit into from
Oct 20, 2022
Merged

Add custom cache on KCP #70

merged 1 commit into from
Oct 20, 2022

Conversation

mmorhun
Copy link
Collaborator

@mmorhun mmorhun commented Oct 20, 2022

Restores some custom cache setting.
The difference with previous version is that all Secrets are excluded from cache (the operator used to cache secrets with part-of appstudio label that were only copies of PaC credentials in local namespace)

Signed-off-by: Mykola Morhun mmorhun@redhat.com

Signed-off-by: Mykola Morhun <mmorhun@redhat.com>
Copy link
Contributor

@gabemontero gabemontero left a comment

Choose a reason for hiding this comment

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

the changes in and of themselves look good to me @mmorhun

I am having a little uncertainty understanding how your comment in the PR description:

The difference with previous version is that all Secrets are excluded from cache (it used to cache secrets with part-of appstudio label that were only copies of PaC credentials in local namespace)

correlates with these changes.

I think I understand, but I'm going to reiterate in my own words what I think you are saying. Please let me know if I have it correct or not.

Are you saying that these changes were needed to exclude all Secrets and that before these changes the excluded object list at https://github.com/redhat-appstudio/build-service/blob/992e47b34fcb9d0c92410f230efdaec3abcb4545/main.go#L223-L226 was NOT sufficient to exclude all Secrets?

If the answer to that question is "yes", then I see you bumped the https://github.com/kcp-dev/controller-runtime dependency. Was the dependency the "it" in your (it used to cache secrets with part-of appstudio label that were only copies of PaC credentials in local namespace statement? And overriding the cache function fixes whatever behavior in KCP lead to secrets getting cached even though secrets are in the exclude list?

Thanks. And if it is easier to sort our my questions over video conf, and then put the answer(s) here do let me know.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 20, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gabemontero, Michkov, mmorhun

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [gabemontero,mmorhun]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@mmorhun
Copy link
Collaborator Author

mmorhun commented Oct 20, 2022

@gabemontero thank you for the review. Sorry, if my quick explanation was not clear.

Are you saying that these changes were needed to exclude all Secrets and that before these changes the excluded object list was NOT sufficient to exclude all Secrets?

No, I am not. The selected code part is enough to exclude all secrets.

I see you bumped the https://github.com/kcp-dev/controller-runtime dependency.

I bumped KCP controller-runtime version because kcp.ClusterAwareBuilderWithOptions was added recently.

the "it" in your "(it used to cache secrets with part-of appstudio label that were only copies of PaC credentials in local namespace" statement?

Sorry for not being clear enough and poor English. By it I meant previous behavior of the operator, so the operator was caching secrets with specific label only, but now we do not cache any secret at all. And this is the difference in the behavior of the operator I wanted to highlight. Changed the description slightly, thanks for pointing out.

And overriding the cache function fixes whatever behavior in KCP lead to secrets getting cached even though secrets are in the exclude list?

Good question. I haven't tried, but requesting controversy behavior is misleading. I think we should be clear in code what we want to have.

Thanks. And if it is easier to sort our my questions over video conf, and then put the answer(s) here do let me know.

We can do both.

@gabemontero
Copy link
Contributor

I'm good @mmorhun those clarifications ^^ helped / I get it now - thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants