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

Add GetNamespaceNames, CreateNamespace and CheckNamespaceExists to resources plugin #3905

Merged
merged 2 commits into from
Dec 10, 2021
Merged

Add GetNamespaceNames, CreateNamespace and CheckNamespaceExists to resources plugin #3905

merged 2 commits into from
Dec 10, 2021

Conversation

absoludity
Copy link
Contributor

Signed-off-by: Michael Nelson minelson@vmware.com

Description of the change

This PR adds the definition only to the resource.proto. Subsequent PRs will add the implementation for the Check and Create methods.

Benefits

Allows implementation of methods required for two call-sites in the dashboard that currently talk directly to the K8s API server.

Possible drawbacks

Applicable issues

Additional information

@absoludity
Copy link
Contributor Author

absoludity commented Dec 6, 2021 •
edited

Trying to find out which upgrade of ts-proto has introduced a change in the generated typescript code that doesn't pass lint (due to not all paths return a value). Seems to have happened recently but not in this PR. Fixed in #3909 and rebased here on that branch.

@absoludity absoludity changed the base branch from master to fix-typescript-proto-gen December 7, 2021 00:44
@absoludity absoludity mentioned this pull request Dec 7, 2021
Merged
Base automatically changed from fix-typescript-proto-gen to master December 9, 2021 01:03
@castelblanque
Copy link
Collaborator

Excellent! Looking forward to having the proto API code automatically generated and out of the PRs 😃

@absoludity
Add GetNamespaceNames, CreateNamespace and CheckNamespaceExists to
43a9068 
resources plugin.

Signed-off-by: Michael Nelson <minelson@vmware.com>
@absoludity
Add an exists field to CheckNamespaceExistsResponse.
ee2f1a3 
Signed-off-by: Michael Nelson <minelson@vmware.com>
@absoludity
Copy link
Contributor Author

Excellent! Looking forward to having the proto API code automatically generated and out of the PRs smiley

Given this change is just the proto (and generated files), and you approved the subsequent one with the implementation, and didn't have any feedback other than the above for this, I'm going to assume that was a +1 :)

@absoludity absoludity merged commit 55aaffb into master Dec 10, 2021
@absoludity absoludity deleted the 3896-namespaces-via-plugin branch December 10, 2021 01:40
@absoludity
Copy link
Contributor Author

Excellent! Looking forward to having the proto API code automatically generated and out of the PRs smiley

Given this change is just the proto (and generated files), and you approved the subsequent one with the implementation, and didn't have any feedback other than the above for this, I'm going to assume that was a +1 :)

Woops, sorry, you hadn't actually added a +1 to the next one either. I'll not land that until then.

castelblanque
castelblanque reviewed Dec 10, 2021
View reviewed changes
dashboard/tsconfig.json Outdated
Comment on lines 33 to 35
/* TODO(minelson): noImplicitReturns disabled until addressed in ts-proto:
* https://github.com/stephenh/ts-proto/issues/432
"noImplicitReturns": true /* Enable error reporting for codepaths that do not explicitly return in a function. * /,*/
Copy link
Collaborator

Choose a reason for hiding this comment

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

Now that ts-proto has fixed the issue, shouldn't we enable this back again?

@castelblanque
Copy link
Collaborator

I had just added a comment which I thought was published, but it was part of the PR review that wasn't sent 😄
Sorry!

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

@castelblanque castelblanque castelblanque left review comments

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

Successfully merging this pull request may close these issues.

None yet
2 participants
@absoludity @castelblanque