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

List automations objects #2765

Merged
merged 13 commits into from Sep 23, 2022
Merged

List automations objects #2765

merged 13 commits into from Sep 23, 2022

Conversation

joshri
Copy link
Contributor

@joshri joshri commented Sep 20, 2022

Closes #2763

This is a branch off of #2751. It extends the useListObjects hook to Automations (Kustomizations and HelmReleases) - this then allowed for a BUNCH of things to be deleted, which caused a bunch of problems, and so on and so on. The main things being deleted are the full kustomization and helmrelease.go files, and the change of the FluxObjectKind type to just Kind, which then removes a bunch of conversion utilities such as addKind and removeKind.

There are probably other things that can be deleted - extra types like Syncable were on my radar. But this is certainly a start.

@joshri joshri marked this pull request as ready for review September 21, 2022 15:15
api/core/core.proto Outdated Show resolved Hide resolved
@joshri joshri requested a review from ozamosi September 22, 2022 13:08
@joshri joshri force-pushed the list-automations-objects branch 2 times, most recently from 8027de2 to 607e3fd Compare September 22, 2022 13:25
@@ -164,18 +164,18 @@ func (cs *coreServer) GetReconciledObjects(ctx context.Context, msg *pb.GetRecon
var opts client.MatchingLabels

switch msg.AutomationKind {
case pb.FluxObjectKind_KindKustomization:
case "Kustomization":
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Here and in a few other places you've introduced strings to represent kind names. I would prefer it if they were using e.g. kustomizev1.KustomizationKind or pb.Kind_Kustomization (or equivalent for other kinds), to avoid the risks of misspellings.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is why it felt weird to me to delete FluxObjectKind all together, since if I do a typed thing here it has to match on the frontend request. If the type field on objects is string and not Kind in the frontend this will not work.......I think? At least that's what I thought was going on while I worked on this. I guess I can convert the string going in into this type and add a slot for "other" or something?

message ObjectRef {
string kind = 1;
Kind kind = 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

❌ I missed this before - this changes the same "can't dynamically add more kinds and have the endpoint work" thing for listEvents that I was mentioning for listObjects and getObject

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same thing here - every time we reference ObjectRef / ClusteredObjectRef in the backend this will have to be a string and not typed?

@joshri
Copy link
Contributor Author

joshri commented Sep 22, 2022

@ozamosi All strings of flux primitives have been changed to their flux controller types - tests are going but things look good from my end. Check it out <|:^)

Robin Sonefors and others added 13 commits September 23, 2022 11:17
This requires adding dedicated inventory handling for helm releases -
rather than redesigning the inventory handling, I added an inventory
field to the object envelope and only set it when the kind is
helmrelease. This isn't good, but also it works and maintains the old
behaviour.

There is one change in behaviour, which is that inventory read
failures from the inventory secret no longer causes the object to
disappear from lists or refuse to render details. It'll now show up,
you just won't see any of the objects it's created. This is required
for #2664 but as listObjects isn't used to list helmReleases yet, it
doesn't actually resolve it.
@joshri joshri merged commit 7283bba into main Sep 23, 2022
@joshri joshri deleted the list-automations-objects branch September 23, 2022 15:27
ozamosi pushed a commit that referenced this pull request Sep 26, 2022
We should only get events for the current cluster - we used to search
all clusters, and if another cluster had the same object then we'd
show incorrect events.

This makes the new, correct object ref kind introduced in #2765 the
only object ref used in our APIs, and then copy-pastes the client
creation code from c3a407a.
ozamosi pushed a commit that referenced this pull request Sep 26, 2022
We should only get events for the current cluster - we used to search
all clusters, and if another cluster had the same object then we'd
show incorrect events.

This makes the new, correct object ref kind introduced in #2765 the
only object ref used in our APIs, and then copy-pastes the client
creation code from c3a407a.
ozamosi pushed a commit that referenced this pull request Sep 26, 2022
We should only get events for the current cluster - we used to search
all clusters, and if another cluster had the same object then we'd
show incorrect events.

This makes the new, correct object ref kind introduced in #2765 the
only object ref used in our APIs, and then copy-pastes the client
creation code from c3a407a.
ozamosi pushed a commit that referenced this pull request Sep 26, 2022
We should only get events for the current cluster - we used to search
all clusters, and if another cluster had the same object then we'd
show incorrect events.

This makes the new, correct object ref kind introduced in #2765 the
only object ref used in our APIs, and then copy-pastes the client
creation code from c3a407a.
ozamosi pushed a commit that referenced this pull request Sep 26, 2022
We should only get events for the current cluster - we used to search
all clusters, and if another cluster had the same object then we'd
show incorrect events.

This makes the new, correct object ref kind introduced in #2765 the
only object ref used in our APIs, and then copy-pastes the client
creation code from c3a407a.
ozamosi pushed a commit that referenced this pull request Sep 27, 2022
We should only get events for the current cluster - we used to search
all clusters, and if another cluster had the same object then we'd
show incorrect events.

This makes the new, correct object ref kind introduced in #2765 the
only object ref used in our APIs, and then copy-pastes the client
creation code from c3a407a.
@ozamosi ozamosi added the type/enhancement New feature or request label Sep 27, 2022
@amulyakish
Copy link

I find it weird that we have deleted so many *_test.go files completely when we still have the corresponding code. I may be missing something here, so let me ask this - we have helm_release.go that was changed as part of this PR, but the corresponding test file helm_release_test.go has been deleted. How are we testing the code in helm_release.go now?

@joshri
Copy link
Contributor Author

joshri commented Sep 27, 2022

Hi @amulyakish!!

The only code remaining in helm_release.go is the getHelmReleaseInventory function, which is tested in objects_test.go. I can see how this might be confusing and I probably should have moved the function into objects.go. Was there any other code you noticed that is remaining from the test files that were deleted? Or any bugs that you've come across since this was merged?

@amulyakish
Copy link

Hi @amulyakish!!

The only code remaining in helm_release.go is the getHelmReleaseInventory function, which is tested in objects_test.go. I can see how this might be confusing and I probably should have moved the function into objects.go. Was there any other code you noticed that is remaining from the test files that were deleted? Or any bugs that you've come across since this was merged?

No I haven't noticed any errors as such. I'm yet to test this in fact. My comment was based only on the list of changes that I saw in this PR. I missed the fact that the getHelmReleaseInventory function is tested in objects_test.go. It is not very obvious unless I search the whole codebase. I think we are good for now. I will report back once I've had a chance to test this. It will probably be after the next release though.

Thank you for all the work you've done here!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove listKustomizations/listHelmReleases
4 participants