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 parseApi to not throw and to match apiVersion more specifically #496

Merged
merged 3 commits into from
Jun 22, 2020

Conversation

Nokel81
Copy link
Collaborator

@Nokel81 Nokel81 commented Jun 18, 2020

Signed-off-by: Sebastian Malton smalton@mirantis.com

This PR removes the explicit throw in parseApi. That function is assumed by several parts of the code to be "no-throw" and to return an empty (ish) object if the passed in URL is not an API URL.

Add a unit test to cover this case.

@Nokel81 Nokel81 added the bug Something isn't working label Jun 18, 2020
@Nokel81 Nokel81 requested review from nevalla, jakolehm and a team June 18, 2020 14:39
@Nokel81 Nokel81 self-assigned this Jun 18, 2020
Signed-off-by: Sebastian Malton <smalton@mirantis.com>
@Nokel81 Nokel81 changed the title remove the explicit throw is new parseApi fix parseApi to not throw and to match apiVersion more specifically Jun 18, 2020
- Add many more unit tests (these are exact copies of the old behaviour
  so that any changes do not change behaviour)

- Make explicit, with documentation, why certain actions are being taken
  with the parsing of the api URL parts

Signed-off-by: Sebastian Malton <smalton@mirantis.com>
@Nokel81
Copy link
Collaborator Author

Nokel81 commented Jun 18, 2020

@nevalla @jakolehm This PR should be the last one (I am so sorry it took so many attempts). I have spent extra time verifying that nothing else is broken (especially side bars) and have added many more unit tests to make sure that any changes I need to make don't change behaviour on the previously working strings.

I guess this goes to show how much testing can be beneficial.

dashboard/client/api/kube-api.ts Outdated Show resolved Hide resolved
@jakolehm jakolehm added this to the 3.5.1 milestone Jun 22, 2020
@nevalla
Copy link
Contributor

nevalla commented Jun 22, 2020

@nevalla @jakolehm This PR should be the last one (I am so sorry it took so many attempts).

No problem at all. Let's make as many attempts as it's required to fix this issue.

Signed-off-by: Sebastian Malton <smalton@mirantis.com>
@Nokel81 Nokel81 merged commit d821b18 into lensapp:master Jun 22, 2020
@ixrock
Copy link
Member

ixrock commented Jun 22, 2020

@Nokel81 this PR seems to break namespace details.
Go to Namespaces -> select namespace -> no details shown at the right.

[apiGroup, apiVersion, resource] = left;
switch (left.length) {
case 2:
resource = left.pop();
Copy link
Member

Choose a reason for hiding this comment

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

Forgot break?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I meant to leave that out, but I will add some more tests once I find which is wrong...

nevalla pushed a commit that referenced this pull request Jun 24, 2020
…496)

* remove the explicit throw is new parseApi

* fix parseApi to be more consitent with the older version

- Add many more unit tests (these are exact copies of the old behaviour
  so that any changes do not change behaviour)

- Make explicit, with documentation, why certain actions are being taken
  with the parsing of the api URL parts

Signed-off-by: Sebastian Malton <smalton@mirantis.com>

Co-authored-by: Sebastian Malton <smalton@mirantis.com>
This was referenced Jun 24, 2020
@jakolehm jakolehm mentioned this pull request Jul 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants