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

Make automation details use getObject instead of dedicated endpoints #2751

Merged
merged 1 commit into from Sep 20, 2022

Conversation

ozamosi
Copy link
Contributor

@ozamosi ozamosi commented Sep 16, 2022

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.

@ozamosi ozamosi requested a review from a team September 16, 2022 14:13
Copy link
Contributor

@joshri joshri left a comment

Choose a reason for hiding this comment

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

Tested locally and both Kustomizations and Helm Releases have their detail pages looking exactly the same as before! Nice tests also - I should add to this for Providers and Alerts

🌞 STUNNING 🌞

import { useToggleSuspend } from "../hooks/flux";
import { useGetObject } from "../hooks/objects";
import { FluxObjectKind } from "../lib/api/core/types.pb";
import { fluxObjectKindToKind } from "../lib/objects";
import { getSourceRefForAutomation } from "../lib/utils";
Copy link
Contributor

@opudrovs opudrovs Sep 16, 2022

Choose a reason for hiding this comment

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

Unused import. If this function is not needed anymore since you added sourceRef getter to the classes, is it possible to remove it completely from utils and tests and replace it with the sourceRef getter in AutomationsTable and SourceDetail too?

Copy link
Contributor

@opudrovs opudrovs Sep 16, 2022

Choose a reason for hiding this comment

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

I see other unused imports, ah, it's because the tests have not completed yet because of the merging/rebase conflict, thought for a moment we broke eslint. 🤣

Copy link
Contributor Author

Choose a reason for hiding this comment

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

is it possible to remove it completely from utils and tests and replace it with the sourceRef getter in AutomationsTable and SourceDetail too?

I left rewriting AutomationsTable out of this PR, so not yet.

This will also apply to FluxObjectKind and everything that uses that, which ripples through the code base, so I cut scope.

@opudrovs
Copy link
Contributor

Tested it locally, works as expected, except for the tests.

@ozamosi ozamosi force-pushed the getobjects-for-automations branch 2 times, most recently from 3cf2d58 to 097a572 Compare September 20, 2022 10:50
@ozamosi ozamosi added the type/enhancement New feature or request label Sep 20, 2022
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.
@ozamosi ozamosi merged commit 6210404 into main Sep 20, 2022
@ozamosi ozamosi deleted the getobjects-for-automations branch September 20, 2022 11:19
@joshri joshri mentioned this pull request Sep 20, 2022
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.

None yet

3 participants