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

Change ordering in some arguments to "cluster, namespace, other args" #2147

Merged
merged 1 commit into from
Nov 5, 2020

Conversation

antgamdia
Copy link
Contributor

Description of the change

cluster, others, namespace and cluster, namespace, others orderings were mixed up across the code. This PR review each file using these methods and changes the order properly. Tests changed accordingly.

Benefits

Now the two first arguments will be cluster, namespace. It was quite odd to be constantly mixing these two orderings and may lead to accidental errors due to a parameter misteaching (note normally they are both string so no type alert will be thrown).

Possible drawbacks

Despite the best effort applied in looking up each method usage, it is slightly possible to have missed some reference.

Applicable issues

Additional information

N/A

"cluster, others, namespace" and "cluster, namespace, others" orderings were mixed up across the code.
Change tests accordingly.
antgamdia added a commit to antgamdia/kubeapps that referenced this pull request Nov 5, 2020
Copy link
Contributor

@andresmgot andresmgot left a comment

Choose a reason for hiding this comment

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

Oof, risky PR this one 😂 It's difficult to notice errors because both parameters (name, namespace) are strings but I didn't notice any (and the tests are passing) so my +1. Thanks!

@antgamdia antgamdia merged commit 953e410 into vmware-tanzu:master Nov 5, 2020
@antgamdia antgamdia deleted the oddMethodOrderingFix branch November 5, 2020 15:13
antgamdia added a commit that referenced this pull request Nov 9, 2020
)

* Add a new API endpoint to force a refresh in a appRepository

Rel #2062
For instance, "POST /api/v1/clusters/default/namespaces/kubeapps/apprepositories/bitnami/refresh" will trigger a refresh in the "bitnami" appRepository in the namespace "kubeapps"

* Change endpoint of appRepository refresh button

Rel #2062
Now it calls the new POST .../refresh endpoint implemented in the backend.

* Change ordering in some arguments to "cluster, namespace, other args"

Rel #2147

* Remove unnecessary namespace check

* Revert unnecessary change

Also fix minor typo

* Remove unnecessary test step

Since refreshing an apprepository no longer requires to get the repo to send an edition later, this test step (faulty) can be removed
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.

None yet

2 participants