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(cli): Prevent Get & Sync from Hanging on Invalid Application Spec #21702

Open
wants to merge 28 commits into
base: master
Choose a base branch
from
Open
Changes from 1 commit
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
858a48b
adding context cancellation to get cmd and cancelling the context bef…
almoelda Jan 29, 2025
af34343
feat: add support for azure workload identity in Microsoft Entra SSO …
jagpreetstamber Jan 29, 2025
c5153c5
fix: Update argo-ui dependency to pull in OCI icon (#18646) (#21698)
keithchong Jan 29, 2025
312dd15
chore(deps): bump github.com/bmatcuk/doublestar/v4 from 4.8.0 to 4.8.…
dependabot[bot] Jan 29, 2025
9ef20a5
chore(deps): bump google.golang.org/protobuf from 1.36.3 to 1.36.4 (#…
dependabot[bot] Jan 29, 2025
9790d93
chore(metrics)!: remove deprecated metrics (#21697)
crenshaw-dev Jan 29, 2025
265536c
codegen
almoelda Jan 29, 2025
ac12366
add cancel call to defer on app get
almoelda Jan 29, 2025
fe3686e
Merge remote-tracking branch 'upstream/master' into cli-hangs-on-inva…
almoelda Jan 29, 2025
564b7cb
app get timeout to uint, app sync do not print after timeout reached
almoelda Jan 29, 2025
8449bab
app get timeout default change to common variable, changed descriptio…
almoelda Jan 29, 2025
693ccd2
Merge remote-tracking branch 'upstream/master' into cli-hangs-on-inva…
almoelda Jan 29, 2025
6f13f63
Update cmd/argocd/commands/app.go
almoelda Feb 1, 2025
0061e3f
codegen
almoelda Feb 2, 2025
e69263c
adding a test and a simulateTimeout to the mock client
almoelda Feb 2, 2025
7c13a30
timeout handling
almoelda Feb 2, 2025
61caf4e
Merge remote-tracking branch 'upstream/master' into cli-hangs-on-inva…
almoelda Feb 2, 2025
74b736e
lose debug code
almoelda Feb 2, 2025
0984859
introduce getAppStateWithRetry to ensure retry without refresh after …
almoelda Feb 2, 2025
9bd679e
revert time sleep in app_test
almoelda Feb 2, 2025
6c69778
introduce lock sync on app in waitOnApplicationStatus
almoelda Feb 2, 2025
a44bde2
fix lock sync on app in waitOnApplicationStatus
almoelda Feb 2, 2025
49ad06f
change app variable inside waitOnApplicationStatus.AfterFunc
almoelda Feb 2, 2025
13e9e8d
remove unnecessary new lines
almoelda Feb 2, 2025
5106b19
lint fix
almoelda Feb 2, 2025
e228de8
Merge remote-tracking branch 'upstream/master' into cli-hangs-on-inva…
almoelda Feb 5, 2025
2b375f5
Merge branch 'master' of https://github.com/argoproj/argo-cd into cli…
almoelda Feb 21, 2025
6b4bdf1
lint fix for AppURL
almoelda Feb 21, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
adding context cancellation to get cmd and cancelling the context bef…
…ore printFinalStatus on sync command

Signed-off-by: Almo Elda <almogldbh@gmail.com>
almoelda committed Jan 29, 2025
commit 858a48bc98a7653714c447cf3fc627b49863fd11
13 changes: 11 additions & 2 deletions cmd/argocd/commands/app.go
Original file line number Diff line number Diff line change
@@ -337,6 +337,7 @@ func NewApplicationGetCommand(clientOpts *argocdclient.ClientOptions) *cobra.Com
refresh bool
hardRefresh bool
output string
timeout int
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
timeout int
timeout uiint

This should be uint.

showParams bool
showOperation bool
appNamespace string
@@ -382,7 +383,7 @@ func NewApplicationGetCommand(clientOpts *argocdclient.ClientOptions) *cobra.Com
`),

Run: func(c *cobra.Command, args []string) {
ctx := c.Context()
ctx, cancel := context.WithCancel(c.Context())
if len(args) == 0 {
c.HelpFunc()(c, args)
os.Exit(1)
@@ -393,6 +394,13 @@ func NewApplicationGetCommand(clientOpts *argocdclient.ClientOptions) *cobra.Com

appName, appNs := argo.ParseFromQualifiedName(args[0], appNamespace)

if timeout != 0 {
time.AfterFunc(time.Duration(timeout)*time.Second, func() {
fmt.Println("Context cancelled due to timeout")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
fmt.Println("Context cancelled due to timeout")

I don't think it's necessary.

cancel()
})
}

app, err := appIf.Get(ctx, &application.ApplicationQuery{
Name: &appName,
Refresh: getRefreshType(refresh, hardRefresh),
@@ -462,6 +470,7 @@ func NewApplicationGetCommand(clientOpts *argocdclient.ClientOptions) *cobra.Com
},
}
command.Flags().StringVarP(&output, "output", "o", "wide", "Output format. One of: json|yaml|wide|tree")
command.Flags().IntVar(&timeout, "timeout", 15, "Specifies the maximum duration for the operation to complete. The command will terminate if the timeout is exceeded.")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
command.Flags().IntVar(&timeout, "timeout", 15, "Specifies the maximum duration for the operation to complete. The command will terminate if the timeout is exceeded.")
command.Flags().UintVar(&timeout, "timeout", defaultCheckTimeoutSeconds, "Time out after this many seconds")

command.Flags().BoolVar(&showOperation, "show-operation", false, "Show application operation")
command.Flags().BoolVar(&showParams, "show-params", false, "Show application parameters and overrides")
command.Flags().BoolVar(&refresh, "refresh", false, "Refresh application data when retrieving")
@@ -2588,8 +2597,8 @@ func waitOnApplicationStatus(ctx context.Context, acdClient argocdclient.Client,
fmt.Println("This is the state of the app after `wait` timed out:")
}

printFinalStatus(app)
cancel()
printFinalStatus(app)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
printFinalStatus(app)
printFinalStatus(app)

why are you printing after the cancel() has been called?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, thanks

Copy link
Member

Choose a reason for hiding this comment

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

This is correct otherwise the ctx never gets called in the Get function. It is better to give a cancellation signal when the timeout has occured.


if printSummary {
fmt.Println()