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

4th step for Investigate and propose package repositories API with similar core interface to packages API. #3496 #4573

Merged

Conversation

gfichtenholt
Copy link
Contributor

@gfichtenholt gfichtenholt commented Apr 8, 2022

4th step for Investigate and propose package repositories API with similar core interface to packages API. #3496

Changed AddPackageRepository and GetPackageRepositoryDetail to reflect latest design spec with user-managed vs kubeapps-managed secrets. For now, store the flag in plugin config, because it is easier to change on demand for unit tests than say, a kubeapps global setting. This is temporary, just until we settle on if/where to hang it.

  • added stub for UpdatePackageRepository
  • bumped flux version
  • added check that desired flux version is installed

Most of the changes are either in generated files or in unit test code

@gfichtenholt gfichtenholt self-assigned this Apr 8, 2022
@gfichtenholt gfichtenholt requested a review from absoludity April 8, 2022 04:43
Copy link
Contributor

@absoludity absoludity left a comment

Choose a reason for hiding this comment

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

I'm struggling to review this one Greg. I've +1'd, but it'd be easier in the future if this was done as 2 or 3 PRs that each focused on just one thing (seemed to be a mix of a few things in this one, including general refactoring).

};
}

// this endpoint only exists for the purpose of integration tests
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 about having an actual extra endpoint for the integration tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, I had a problem. I have a bunch of integration tests for AddRepository and GetRepositoryDetail. Once I split the server-side functionality of user-managed secrets and kubeapps-managed secrets a number of tests became invalid. For example, I had a test that would GetRepositoryDetail and return detail with secret refs. That only works with user-managed secrets. kubeapps-managed secrets would return Username/Password, etc. So I wanted the flexibility of switching from user-managed to kubeapps-managed and back while the server is running so that I can have a new test that returns Username/Password and the existing test that returns secretRefs. All while the server is running. My only way to do that is via a gRPC call. That's why the endpoint. The endpoint is only in the flux-specific API, BTW, not in the core API. In the future we may change how the flag works, so I may get rid of the endpoint

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, fine for now. If we need this kind of thing longer term, we could probably have a test plugin for example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

@gfichtenholt
Copy link
Contributor Author

I'm struggling to review this one Greg. I've +1'd, but it'd be easier in the future if this was done as 2 or 3 PRs that each focused on just one thing (seemed to be a mix of a few things in this one, including general refactoring).

sorry about that Michael, I didn't think it would be that difficult to review. Really the there are 2 things in there: 1) separate the functionality of user-managed vs kubeapps-managed secrets based on a flag like we discussed and 2) just added empty stub for UpdateRepository (no implementation for plug in, just the core).

Once again, I will try to do better next time

@absoludity
Copy link
Contributor

I'm struggling to review this one Greg. I've +1'd, but it'd be easier in the future if this was done as 2 or 3 PRs that each focused on just one thing (seemed to be a mix of a few things in this one, including general refactoring).

sorry about that Michael, I didn't think it would be that difficult to review. Really the there are 2 things in there: 1) separate the functionality of user-managed vs kubeapps-managed secrets based on a flag like we discussed and 2) just added empty stub for UpdateRepository (no implementation for plug in, just the core).

Oh - I didn't isolate it into those two when going through - seemed to be other refactoring too, which I wasn't sure whether it was changing code (tracking where it's deleted and added etc.)

Once again, I will try to do better next time

Heh - it's not that big a problem, no need for penance :P, I'm just keen to be able to review things properly and find it difficult when it's not clear to me what has changed. I should have re-read the PR description after reading through, as you'd identified what had changed pretty clearly there.

@gfichtenholt gfichtenholt merged commit 02252ac into vmware-tanzu:main Apr 11, 2022
@gfichtenholt gfichtenholt deleted the further-fluxv2-plugin-features-37 branch April 11, 2022 05:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Investigate and propose package repositories API with similar core interface to packages API.
2 participants