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

feat(cli): add support for multiple sources to app diff|manifests command with revisions flag #17650

Merged
merged 9 commits into from
Apr 3, 2024
Merged

feat(cli): add support for multiple sources to app diff|manifests command with revisions flag #17650

merged 9 commits into from
Apr 3, 2024

Conversation

ishitasequeira
Copy link
Member

@ishitasequeira ishitasequeira commented Mar 28, 2024

The PR adds support for multiple sources to argocd app manifests and argocd app diff commands. As part of this PR, the support for multiple sources is by the addition of 2 new flags --revisions and --source-indexes. This 2 flags allow the user to set a revision for a specific source defined by index in source-indexes flag.

Examples:

  1. argocd app manifests command
./dist/argocd app manifests guestbook --revisions 4171e73dec711bde9a130398d3cfc93363b16339 --source-indexes 1
  1. argocd app diff command
./dist/argocd app diff guestbook --revisions 4171e73dec711bde9a130398d3cfc93363b16339 --source-indexes 1

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).
  • The title of the PR conforms to the Toolchain Guide
  • 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.
  • 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).
  • My new feature complies with the feature status guidelines.
  • I have added a brief description of why this PR is necessary and/or what this PR solves.
  • Optional. My organization is added to USERS.md.
  • Optional. For bug fixes, I've indicated what older releases this fix should be cherry-picked into (this may or may not happen depending on risk/complexity).

@ishitasequeira ishitasequeira marked this pull request as ready for review March 28, 2024 04:45
@ishitasequeira ishitasequeira requested review from a team as code owners March 28, 2024 04:45
@pasha-codefresh
Copy link
Member

@CodiumAI-Agent /review

@CodiumAI-Agent
Copy link

PR Review

⏱️ Estimated effort to review [1-5]

4, due to the complexity introduced by handling multiple sources and revisions, and the significant changes made across multiple files including server logic, CLI commands, and documentation. The PR touches on critical functionalities and requires careful consideration of edge cases and error handling.

🧪 Relevant tests

No

🔍 Possible issues

Possible Bug: The implementation assumes sourceIndexes start at 1, which is unconventional and might lead to off-by-one errors or confusion. Consider using zero-based indexing for consistency with common programming practices.

Error Handling: There's a lack of detailed error messages in some new conditions, such as when source-indexes and revisions lengths do not match. Providing more context in error messages can improve the user experience and make debugging easier.

Performance Concern: The loop that generates manifests for each source could potentially lead to performance issues for applications with a large number of sources. Consider optimizing this part of the code or documenting potential performance impacts.

🔒 Security concerns

No

Code feedback:
relevant filecmd/argocd/commands/app.go
suggestion      

Consider using zero-based indexing for sourceIndexes to align with common programming practices and reduce the risk of off-by-one errors. This change would involve adjusting the validation logic and documentation to reflect the zero-based approach. [important]

relevant lineerrors.CheckError(fmt.Errorf("source-index cannot be less than or equal to 0. Index starts at 1."))

relevant fileserver/application/application.go
suggestion      

Optimize the loop that generates manifests for each source to handle cases with a large number of sources more efficiently. This could involve parallel processing or other optimization techniques to reduce the potential performance impact. [medium]

relevant linefor _, source := range sources {

relevant filecmd/argocd/commands/app.go
suggestion      

Improve error messages to provide more context, especially when the lengths of revisions and sourceIndexes do not match. This will help users understand the issue more clearly and fix their commands accordingly. [medium]

relevant lineerrors.CheckError(fmt.Errorf("While using revisions and source-indexes, length of values for both flags should be same."))

relevant fileserver/application/application.go
suggestion      

Add validation to ensure that the revisionSourceMappings map does not contain invalid indexes before proceeding with the generation of manifests. This can prevent runtime errors and ensure that the mappings are correctly applied. [important]

relevant lineif q.GetRevisionSourceMappings() != nil && len(q.GetRevisionSourceMappings()) > 0 {


✨ Review tool usage guide:

Overview:
The review tool scans the PR code changes, and generates a PR review which includes several types of feedbacks, such as possible PR issues, security threats and relevant test in the PR. More feedbacks can be added by configuring the tool.

The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on any PR.

  • When commenting, to edit configurations related to the review tool (pr_reviewer section), use the following template:
/review --pr_reviewer.some_config1=... --pr_reviewer.some_config2=...
[pr_reviewer]
some_config1=...
some_config2=...

See the review usage page for a comprehensive guide on using this tool.

Copy link
Member

@agaudreault agaudreault left a comment

Choose a reason for hiding this comment

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

I like the input parameters, seems like a good and intuitive approach for people using multi sources

cmd/argocd/commands/app.go Show resolved Hide resolved
--server-side-generate Used with --local, this will send your manifests to the server for diffing
--source-indexes int64Slice List of source indexes. Default is empty array. Indexes start at 1. (default [])
Copy link
Member

Choose a reason for hiding this comment

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

intArray type would make it clearer to users and not leak go implementation details

Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately, this is an auto-generated document. I am using int64Slice datatype for sourceIndexes as we do not have a function to parse int[] input in flagSet.

go.mod Outdated
@@ -94,7 +94,7 @@ require (
gopkg.in/yaml.v3 v3.0.1
k8s.io/api v0.26.11
k8s.io/apiextensions-apiserver v0.26.10
k8s.io/apimachinery v0.26.11
k8s.io/apimachinery v0.29.3
Copy link
Member

Choose a reason for hiding this comment

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

Is that necessary in this PR? IIRC, updating this affect the kubernetes compatibility. It's probably better to update all k8s.io dependency to be in v0.29 in a dedicated PR if necessary.

Copy link
Member Author

Choose a reason for hiding this comment

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

Will revert this.

Signed-off-by: ishitasequeira <[email protected]>
Signed-off-by: ishitasequeira <[email protected]>
Signed-off-by: ishitasequeira <[email protected]>
Signed-off-by: ishitasequeira <[email protected]>
Signed-off-by: ishitasequeira <[email protected]>
@@ -1125,6 +1125,8 @@ func NewApplicationDiffCommand(clientOpts *argocdclient.ClientOptions) *cobra.Co
serverSideGenerate bool
localIncludes []string
appNamespace string
revisions []string
sourceIndexes []int64
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be int64 ? I feel we can use uint or unit32 here as the index positions do not have negative numbers.

Copy link
Member Author

@ishitasequeira ishitasequeira Apr 2, 2024

Choose a reason for hiding this comment

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

The command flagset does not support uint or uint32. Thus, had to keep it int or int64. Had used int64 to keep it consistent with what is needed by revisionSourceMappings. I can change this to int and then typecast it to int64 for creating revisionSourceMappings.

Copy link
Contributor

Choose a reason for hiding this comment

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

If flagset support is missing for uint, then let's stick to int64

Copy link
Contributor

@anandf anandf left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@agaudreault agaudreault left a comment

Choose a reason for hiding this comment

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

LGTM. Lets discuss the index at the next contributor meeting

Copy link
Member

@crenshaw-dev crenshaw-dev left a comment

Choose a reason for hiding this comment

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

Can you add a Long usage description for the app manifests command to provide an example of how to use this?

I'm not sure the docs are correct about the flags being comma-delimited:

It's behaving like it's not comma-delimited but rather repeated:
dist/argocd app manifests test --revisions d7927a27b4533926b7d86b5f249cd9ebe7625e90,53e28ff20cc530b9ada2173fbbd64d48338583ba --source-indexes 1,2
FATA[0000] While using revisions and source-indexes, length of values for both flags should be same.

This seems to work:

dist/argocd app manifests test --revisions d7927a27b4533926b7d86b5f249cd9ebe7625e90 --revisions 53e28ff20cc530b9ada2173fbbd64d48338583ba --source-indexes 1 --source-indexes 2

Should we add an e2e test to make sure the usage is tested?

Signed-off-by: ishitasequeira <[email protected]>
@ishitasequeira
Copy link
Member Author

ishitasequeira commented Apr 3, 2024

@crenshaw-dev , thanks for testing that out. I updated the doc to remove comma-separated and added example for manifests command.

Should we add an e2e test to make sure the usage is tested?

Unfortunately, the current setup for e2e tests, would not work for cli commands of multiple sources.

Copy link
Member

@crenshaw-dev crenshaw-dev left a comment

Choose a reason for hiding this comment

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

LGTM!

@crenshaw-dev crenshaw-dev enabled auto-merge (squash) April 3, 2024 17:39
@crenshaw-dev crenshaw-dev merged commit 4b11524 into argoproj:master Apr 3, 2024
27 checks passed
mkieweg pushed a commit to mkieweg/argo-cd that referenced this pull request Jun 11, 2024
…mand with `revisions` flag (argoproj#17650)

* Add support for multiple source to manifests --revision command

Signed-off-by: ishitasequeira <[email protected]>

* Update GetManifests to support multiple sources

Signed-off-by: ishitasequeira <[email protected]>

* remove testing logs

Signed-off-by: ishitasequeira <[email protected]>

* update cli docs

Signed-off-by: ishitasequeira <[email protected]>

* add extra validation for diff command

Signed-off-by: ishitasequeira <[email protected]>

* fix lint

Signed-off-by: ishitasequeira <[email protected]>

* Empty-Commit

Signed-off-by: ishitasequeira <[email protected]>

* revert apimachinery version

Signed-off-by: ishitasequeira <[email protected]>

* Update docs based on comments

Signed-off-by: ishitasequeira <[email protected]>

---------

Signed-off-by: ishitasequeira <[email protected]>
Hariharasuthan99 pushed a commit to AmadeusITGroup/argo-cd that referenced this pull request Jun 16, 2024
…mand with `revisions` flag (argoproj#17650)

* Add support for multiple source to manifests --revision command

Signed-off-by: ishitasequeira <[email protected]>

* Update GetManifests to support multiple sources

Signed-off-by: ishitasequeira <[email protected]>

* remove testing logs

Signed-off-by: ishitasequeira <[email protected]>

* update cli docs

Signed-off-by: ishitasequeira <[email protected]>

* add extra validation for diff command

Signed-off-by: ishitasequeira <[email protected]>

* fix lint

Signed-off-by: ishitasequeira <[email protected]>

* Empty-Commit

Signed-off-by: ishitasequeira <[email protected]>

* revert apimachinery version

Signed-off-by: ishitasequeira <[email protected]>

* Update docs based on comments

Signed-off-by: ishitasequeira <[email protected]>

---------

Signed-off-by: ishitasequeira <[email protected]>
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.

6 participants