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

OCM-6407 | edit option sends an empty payload #601

Merged
merged 1 commit into from
Mar 11, 2024

Conversation

ahitacat
Copy link
Contributor

@ahitacat ahitacat commented Feb 28, 2024

EDIT

This commit adds a parameter when creating the command that check for a minimum amount of arguments. The
command will fail if no parameters are used.

@tzvatot
Copy link
Contributor

tzvatot commented Feb 28, 2024

I suggest to use cmd.Flags().NFlag() and if it's < 1 then print an INFO message that no change is done.

@ahitacat
Copy link
Contributor Author

ahitacat commented Feb 28, 2024

I see there is also used in this cli tool the builtin args validator.

var Cmd = &cobra.Command{
	...
	Args: cobra.MinimumNArgs(2),
}

If I remove the clusterNeedsUpdate bool, I see a possible problem when the --enable-delete-protection parameter is added. As that parameter doesn't require to update the cluster.

@tzvatot
Copy link
Contributor

tzvatot commented Feb 29, 2024

I see there is also used in this cli tool the builtin args validator.

var Cmd = &cobra.Command{
	...
	Args: cobra.MinimumNArgs(2),
}

If I remove the clusterNeedsUpdate bool, I see a possible problem when the --enable-delete-protection parameter is added. As that parameter doesn't require to update the cluster.

Why not? it requires a patch to the /delete_protection endpoint. It's not the cluster's endpoint, but it's still semantically updating the cluster.

This commit adds a parameter when creating the command
that check for a minimum amount of arguments. The
command will fail if no parameters are used.

Signed-off-by: Alba Hita Catala <[email protected]>
@mnecas
Copy link
Collaborator

mnecas commented Mar 4, 2024

Agree with @ahitacat in case we will use the --enable-delete-protection without adding any check to [1] we will still send empty patch request to the /clusters/{id} endpoint and patch to update the delete-protection to /clusters/{id}/delete_protection. This could lead to unexpected behaviour as we would not update anything in the /clusters/{id} but still could fail on permission check.

[1]

err = c.UpdateCluster(clusterCollection, cluster.ID(), clusterConfig)

@mnecas
Copy link
Collaborator

mnecas commented Mar 4, 2024

Ah did not know about the #603 should be good.
lgmt

Copy link
Contributor

@tzvatot tzvatot left a comment

Choose a reason for hiding this comment

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

LGTM

@tzvatot tzvatot merged commit 9451a5b into openshift-online:main Mar 11, 2024
4 checks passed
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.

3 participants