-
Notifications
You must be signed in to change notification settings - Fork 5.2k
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: Update command argocd app history to support multiple sources #17530
feat: Update command argocd app history to support multiple sources #17530
Conversation
… sources along with all the REVISIONS Signed-off-by: Mangaal <angommeeteimangaal@gmail.com>
cmd/argocd/commands/app.go
Outdated
if depInfo.Source.RepoURL != "" { | ||
rev := depInfo.Source.TargetRevision | ||
if len(depInfo.Revision) >= 7 { | ||
rev = fmt.Sprintf("%s (%s)", rev, depInfo.Revision[0:7]) | ||
} | ||
varHistory[depInfo.Source.RepoURL] = append(varHistory[depInfo.Source.RepoURL], history{ | ||
id: depInfo.ID, | ||
date: depInfo.DeployedAt.String(), | ||
revision: rev, | ||
}) | ||
} | ||
if depInfo.Sources != nil { | ||
for i, sourceInfo := range depInfo.Sources { | ||
rev := sourceInfo.TargetRevision | ||
if len(depInfo.Revisions) == len(depInfo.Sources) && len(depInfo.Revisions[i]) >= 7 { | ||
rev = fmt.Sprintf("%s (%s)", rev, depInfo.Revisions[i][0:7]) | ||
} | ||
varHistory[sourceInfo.RepoURL] = append(varHistory[sourceInfo.RepoURL], history{ | ||
id: depInfo.ID, | ||
date: depInfo.DeployedAt.String(), | ||
revision: rev, | ||
}) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As Sources takes priority over Source, the if clause should be something like below:
if depInfo.Sources != nil {
...
} else {
...
}
} | ||
} | ||
|
||
func TestPrintApplicationHistoryTableWithMultipleSources(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's also add a test where the application has both source and sources, but the history command only displays history of sources.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @ishitasequeira , I have updated the PR with the changes you have suggested.
…om/Mangaal/argo-cd into feat/upadte-cli-argocd-app-history
cmd/argocd/commands/app.go
Outdated
rev = fmt.Sprintf("%s (%s)", rev, depInfo.Revision[0:7]) | ||
if depInfo.Source.RepoURL != "" { | ||
rev := depInfo.Source.TargetRevision | ||
if len(depInfo.Revision) >= 7 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we avoid this magic number 7
and create a constant like MAX_ALLOWED_REVISIONS
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @anandf , I have removed the no 7 and added a var MAX_ALLOWED_REVISIONS
…to overlooked source if sources is persent Signed-off-by: Mangaal <angommeeteimangaal@gmail.com>
Signed-off-by: Mangaal <angommeeteimangaal@gmail.com>
…om/Mangaal/argo-cd into feat/upadte-cli-argocd-app-history
cmd/argocd/commands/app_test.go
Outdated
} | ||
} | ||
|
||
func TestPrintApplicationHistoryTableForWhenBothSourcesAndSourceFiledsExist(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The testcase mentions source and sources field but it seems we covered it in above test case. Maybe we can remove this testcase. Rest of the PR looks good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @ishitasequeira , I have removed the extra unit test.
…om/Mangaal/argo-cd into feat/upadte-cli-argocd-app-history
Signed-off-by: Mangaal <angommeeteimangaal@gmail.com>
…rcesAndSourceFiledsExist() Signed-off-by: Mangaal <angommeeteimangaal@gmail.com>
…om/Mangaal/argo-cd into feat/upadte-cli-argocd-app-history
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @Mangaal, the PR LGTM
…rgoproj#17530) * update argocd app history command to print app history group by thier sources along with all the REVISIONS Signed-off-by: Mangaal <angommeeteimangaal@gmail.com> * upadte unit test to ahve both Source and Sources and update function to overlooked source if sources is persent Signed-off-by: Mangaal <angommeeteimangaal@gmail.com> * remove magic no 7 and introduc a variable MAX_ALLOWED_REVISIONS Signed-off-by: Mangaal <angommeeteimangaal@gmail.com> * remove extra unit test Signed-off-by: Mangaal <angommeeteimangaal@gmail.com> * remove extra unit test TestPrintApplicationHistoryTableForWhenBothSourcesAndSourceFiledsExist() Signed-off-by: Mangaal <angommeeteimangaal@gmail.com> --------- Signed-off-by: Mangaal <angommeeteimangaal@gmail.com> Co-authored-by: Ishita Sequeira <46771830+ishitasequeira@users.noreply.github.com> Signed-off-by: Kevin Lyda <kevin@lyda.ie>
…rgoproj#17530) * update argocd app history command to print app history group by thier sources along with all the REVISIONS Signed-off-by: Mangaal <angommeeteimangaal@gmail.com> * upadte unit test to ahve both Source and Sources and update function to overlooked source if sources is persent Signed-off-by: Mangaal <angommeeteimangaal@gmail.com> * remove magic no 7 and introduc a variable MAX_ALLOWED_REVISIONS Signed-off-by: Mangaal <angommeeteimangaal@gmail.com> * remove extra unit test Signed-off-by: Mangaal <angommeeteimangaal@gmail.com> * remove extra unit test TestPrintApplicationHistoryTableForWhenBothSourcesAndSourceFiledsExist() Signed-off-by: Mangaal <angommeeteimangaal@gmail.com> --------- Signed-off-by: Mangaal <angommeeteimangaal@gmail.com> Co-authored-by: Ishita Sequeira <46771830+ishitasequeira@users.noreply.github.com>
…rgoproj#17530) * update argocd app history command to print app history group by thier sources along with all the REVISIONS Signed-off-by: Mangaal <angommeeteimangaal@gmail.com> * upadte unit test to ahve both Source and Sources and update function to overlooked source if sources is persent Signed-off-by: Mangaal <angommeeteimangaal@gmail.com> * remove magic no 7 and introduc a variable MAX_ALLOWED_REVISIONS Signed-off-by: Mangaal <angommeeteimangaal@gmail.com> * remove extra unit test Signed-off-by: Mangaal <angommeeteimangaal@gmail.com> * remove extra unit test TestPrintApplicationHistoryTableForWhenBothSourcesAndSourceFiledsExist() Signed-off-by: Mangaal <angommeeteimangaal@gmail.com> --------- Signed-off-by: Mangaal <angommeeteimangaal@gmail.com> Co-authored-by: Ishita Sequeira <46771830+ishitasequeira@users.noreply.github.com>
Description
This PR update the
argocd app history
command to display the history of an application grouped by their sources. The updated output now includes the source URL, along with the application history details such as ID, date, and revision.Changes Made
argocd app history
command to group the application history by source.Previous command output
Output after changes
Checklist: