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

Add HomeSpace status field #384

Merged
merged 3 commits into from
Oct 18, 2023
Merged

Conversation

baijum
Copy link
Contributor

@baijum baijum commented Oct 18, 2023

Description

This field is going to be used by the proxy when no workspace context is provided.

Ref. Ref. https://issues.redhat.com/browse/SANDBOX-425

Checks

  1. Did you run make generate target? yes

  2. Did make generate change anything in other projects (host-operator, member-operator)? yes

  3. In case of new CRD, did you the following? no

  4. In case other projects are changed, please provides PR links.

This field is going to be used by the proxy when no workspace
context is provided.

Signed-off-by: Baiju Muthukadan <[email protected]>
Copy link
Contributor

@mfrancisc mfrancisc left a comment

Choose a reason for hiding this comment

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

Looks good 👍

I have one minor comment.

@@ -265,6 +265,11 @@ type UserSignupStatus struct {
// CompliantUsername is used to store the transformed, DNS-1123 compliant username
// +optional
CompliantUsername string `json:"compliantUsername,omitempty"`

// HomeSpace represents the default home space for the user.
Copy link
Contributor

Choose a reason for hiding this comment

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

minor thing - maybe we could specify that this field will contain the name of the Space that is created for the user immediately after their account is approved. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the review!
Done. ab5bad2

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks!

It seems like the the generated.openapi.go file still has the old description for some reason : https://github.com/codeready-toolchain/api/pull/384/files#diff-72ca28827e299dac63194a397a04873f7f4d9fe3a216136c97a81c5e561d92acR4931

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah. I missed copying that from the generated location. I had to copy it like this:
cp github.com/codeready-toolchain/api/api/v1alpha1/zz_generated.openapi.go api/v1alpha1/

Fixed here: bf58427

Copy link
Contributor

@mfrancisc mfrancisc Oct 18, 2023

Choose a reason for hiding this comment

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

you can avoid the copy if you clone the projects into your GOPATH location for example:
GOPATH="~/go"
and projects should be located at:
~/go/src/github.com/codeready-toolchain/api
same location for the other toolchain projects.

Signed-off-by: Baiju Muthukadan <[email protected]>
Signed-off-by: Baiju Muthukadan <[email protected]>
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

Copy link
Contributor

@mfrancisc mfrancisc left a comment

Choose a reason for hiding this comment

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

Thanks for addressing my comments!

@baijum baijum merged commit a200488 into codeready-toolchain:master Oct 18, 2023
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