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.

Sign up for GitHub

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

Jump to bottom

Fix issue in extraCommandArgs field #1541

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

Fix issue in extraCommandArgs field #1541

wants to merge 2 commits into from
+105 −15

Conversation

Rizwana777
Copy link
Contributor

@Rizwana777 Rizwana777 commented Sep 9, 2024 •
edited
Loading

@Rizwana777 Rizwana777 force-pushed the issue-5200 branch 6 times, most recently from c2b7eb0 to e8ed105 Compare September 9, 2024 14:20
saumeya
saumeya reviewed Sep 11, 2024
View reviewed changes
controllers/argocd/deployment.go Outdated
// If this flag already exists in cmd, skip it
if _, exists := existingArgs[arg]; exists {
if i+1 < len(extraArgs) && !strings.HasPrefix(extraArgs[i+1], "--") {
i++ // Skip the associated value
Copy link
Collaborator

Choose a reason for hiding this comment

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

What if an existing flag is added with a new value, will that also be skipped?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, it is skipped

Copy link
Collaborator

@saumeya saumeya Sep 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Is that the expected behavior? what if the user wants to change the current flag value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

based on the existing unit test https://github.com/argoproj-labs/argocd-operator/blob/master/controllers/argocd/deployment_test.go#L1304 which is updating existing cmd i.e --redis command, I think this is expected behaviour, I might be wrong and open for suggestions

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, maybe we can confirm with others as well. Otherwise looks good.

Copy link
Collaborator

@svghadi svghadi Oct 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think users should be allowed to override default flags. There may be cases where tuning of default params could be useful.

jopit
jopit requested changes Sep 20, 2024
View reviewed changes
Copy link
Collaborator

@jopit jopit left a comment

Choose a reason for hiding this comment

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

I don't see any logic to address the 3rd problem reported in the Jira issue

3. Adding argument and value as a single element causes the pod to crash

Other than that, it looks good to me.

@Rizwana777
Copy link
Contributor Author

Rizwana777 commented Sep 23, 2024 •
edited
Loading

I don't see any logic to address the 3rd problem reported in the Jira issue

3. Adding argument and value as a single element causes the pod to crash

Other than that, it looks good to me.

Thanks John, for 3rd point -

Pod crashes as mentioned in description because ,

'{.spec.server.extraCommandArgs}' ["foo bar"] , here extraCommandArgs is of type []string and we need to pass the flag value i.e, foo as --foo and bar as value like this ["--foo" "bar"] , if we pass values as mentioned pod will not crash, 
and pod crashes as mentioned in description is :
error : Error: unknown flag: --foo
Usage:
argocd-server [flags]
argocd-server [command], as --foo is a flag and we can use available flags

@Rizwana777
Copy link
Contributor Author

FAIL: kuttl/harness/1-036_validate_role_rolebinding_for_source_namespace this e2e test is failing in other PRs as well

@saumeya
Copy link
Collaborator

saumeya commented Sep 24, 2024

Re-ran the tests

@Rizwana777
Copy link
Contributor Author

FAIL: kuttl/harness/1-036_validate_role_rolebinding_for_source_namespace this e2e test is failing in other PRs as well

All the tests are passing now

svghadi
svghadi requested changes Nov 4, 2024
View reviewed changes
Copy link
Collaborator

@svghadi svghadi left a comment

Choose a reason for hiding this comment

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

@Rizwana777 - Can we allow users to override params? There may be cases where tuning of default params could be useful.

@Rizwana777 Rizwana777 force-pushed the issue-5200 branch 2 times, most recently from 14f8f29 to 9624248 Compare November 8, 2024 12:39
@Rizwana777
Copy link
Contributor Author

@svghadi I have updated the code which allow the users to override existing params, PTAL

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

@saumeya saumeya saumeya left review comments

@svghadi svghadi svghadi requested changes

@jopit jopit jopit requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Rizwana777 @saumeya @jopit @svghadi