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

fix(ui): pass getResource function to the extension component #8617

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

yq314
Copy link

@yq314 yq314 commented Feb 28, 2022

This is an attempt to address argoproj-labs/argocd-extensions#10

When workloadRef is used in Rollout resource, the extension is not able to access spec.template as it's defined in the Deployment resource. We need additional call to the api to fetch necessary information from Deployment.

The fix is to pass the getResource() function to the extension component.

I clearly miss a lot of contexts and background knowledge of the project, let me know if the attempt doesn't make sense and I'm happy to update the implementation.

Note on DCO:

If the DCO action in the integration test fails, one or more of your commits are not signed off. Please click on the Details link next to the DCO action for instructions on how to resolve this.

Checklist:

  • Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this does not need to be in the release notes.
  • The title of the PR states what changed and the related issues number (used for the release note).
  • I've included "Closes [ISSUE #]" or "Fixes [ISSUE #]" in the description to automatically close the associated issue.
  • I've updated both the CLI and UI to expose my feature, or I plan to submit a second PR with them.
  • Does this PR require documentation updates?
  • I've updated documentation as required by this PR.
  • Optional. My organization is added to USERS.md.
  • I have signed off all my commits as required by DCO
  • I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
  • My build is green (troubleshooting builds).

Signed-off-by: Qing Ye <ye.qing+gitlab@gojek.com>
@yq314
Copy link
Author

yq314 commented Feb 28, 2022

@rbreeze @alexmt could you take a look thanks

Signed-off-by: Qing Ye <ye.qing+gitlab@gojek.com>
@codecov
Copy link

codecov bot commented Feb 28, 2022

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (0a46d37) 42.64% compared to head (97cd4ab) 42.66%.
Report is 2312 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #8617      +/-   ##
==========================================
+ Coverage   42.64%   42.66%   +0.01%     
==========================================
  Files         184      184              
  Lines       23130    23130              
==========================================
+ Hits         9864     9868       +4     
+ Misses      11862    11859       -3     
+ Partials     1404     1403       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -122,7 +122,7 @@ export const ResourceDetails = (props: ResourceDetailsProps) => {
key: 'extension',
content: (
<ErrorBoundary message={`Something went wrong with Extension for ${state.kind}`}>
<ExtensionComponent tree={tree} resource={state} />
<ExtensionComponent tree={tree} resource={state} getResourceFunc={resource => services.applications.getResource(application.metadata.name, resource)} />
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure it's best to encourage the extension to request any resource from the API.

However, I see why you took this approach, as Argo CD cannot predict what other resources the extension might need. The extension already receives the tree data, which contains all the necessary resources (including a Deployment referenced by workloadRef).

Two alternative approaches:

  1. Pass a map of resources instead of a function here, such that accessing the map at resourceMap[resource] returns the same thing as services.applications.getResource(application.metadata.name, resource)
  2. Don't make any changes to Argo CD, but rather change the Rollout extension to parse the tree for the relevant deployment when a workloadRef is being used

I prefer the second solution, as the Rollout extension already has all the data it needs. @alexmt what are your thoughts?

Copy link
Author

Choose a reason for hiding this comment

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

@rbreeze the second solution was actually the first approach I attempted, however, the tree doesn't have all the information we need.

This is the deployment in tree:

{
  createdAt: "2022-03-01T11:03:34Z",
  group: "apps",
  health: { status: "Healthy" },
  info: [{ name: "Revision", value: "Rev:1" }],
  kind: "Deployment",
  name: "pice-service",
  namespace: "pricing",
  resourceVersion: "71769635",
  uid: "bbffaa75-ae9f-44da-88d9-a817fa0fd70d",
  version: "v1",
}

We still don't have access to the spec of deployment. We do need the information in tree to call the getResource() function.

It looks like solution 1 is more viable, or can we actually just enrich the items in the tree with what is returned in services.applications.getResource(application.metadata.name, resource) ?

@mihstaub
Copy link

Hey guys,

Any update on this pr?

@todaywasawesome todaywasawesome added the lifecycle/rotten Issue/PR had no activity for a long time and should be closed soon label Feb 15, 2024
@christianh814
Copy link
Member

Hey guys,

Any update on this pr?

Looks like we're still waiting for @yq314 and @rbreeze to resolve conflicts

@kencochrane
Copy link

@rbreeze, do you have a few minutes to help with this? If not, let us know, and we can see if we can find someone else.

@rbreeze
Copy link
Member

rbreeze commented Jun 24, 2024

@kencochrane Sure thing, I should have some time this week

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lifecycle/rotten Issue/PR had no activity for a long time and should be closed soon
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants