Skip to content

added delete fucntionality - #234

Open
nishita-lab wants to merge 1 commit into
walmartlabs:masterfrom
nishita-lab:deleteParam
Open

added delete fucntionality#234
nishita-lab wants to merge 1 commit into
walmartlabs:masterfrom
nishita-lab:deleteParam

Conversation

@nishita-lab

Copy link
Copy Markdown

No description provided.

@CLAassistant

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

Comment on lines +392 to +394
String forceRefresh();

String appProject();

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

These should be @Nullable (or have a default value).

Comment on lines +399 to +404
if (!deleteParm.forceRefresh().isEmpty() && !deleteParm.forceRefresh().isBlank()) {
builder.header("forceRefresh", deleteParm.forceRefresh());
}
if (!deleteParm.appProject().isEmpty() && !deleteParm.appProject().isBlank()) {
builder.header("appProject", deleteParm.appProject());
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Remove isEmpty() check. Checking isBlank() covers both cases.

Comment on lines +399 to +402
if (!deleteParm.forceRefresh().isEmpty() && !deleteParm.forceRefresh().isBlank()) {
builder.header("forceRefresh", deleteParm.forceRefresh());
}
if (!deleteParm.appProject().isEmpty() && !deleteParm.appProject().isBlank()) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Need a null check for both forceRefresh() and appProject().

Comment thread tasks/argocd/argocd.yml
Comment on lines +1966 to +1974
- type: string
description: whether to force a cache refresh on repo's connection state
name: forceRefresh
in: query
required: true
- type: string
description: App project for query
name: appProject
in: query

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This doesn't seem like the correct location for manually adding these params to the spec. These params aren't in an up-to-date spec that I can find.

Comment on lines +392 to +394
String forceRefresh();

String appProject();

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'm not sure these params make sense here. They're not part of the api spec, and why would something need to be refreshed when the application is deleted?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants