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

ASC-402: Modify NSTemplateTierSpec to support SA token copy #380

Merged
merged 11 commits into from
Oct 17, 2023

Conversation

Kartikey-star
Copy link
Contributor

@Kartikey-star Kartikey-star commented Oct 12, 2023

Description

A few sentences describing the overall goals of the pull request's commits.

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? yes/no

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

Signed-off-by: Kartikey Mamgain <[email protected]>
Signed-off-by: Kartikey Mamgain <[email protected]>
Copy link
Contributor

@MatousJobanek MatousJobanek left a comment

Choose a reason for hiding this comment

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

I don't see any of the generated files to be modified as part of this PR - when you run make generate then it should update zz_generated.deepcopy.go zz_generated.openapi.go (doesn't have to be both - it depends on the type of the changes)

Comment on lines 25 to 28

// flag to signify whether to copy SA token or not
// +optional
CopySaToken bool `json:"copySaToken,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a SpaceRequest specific configuration (it's used only when the Space is created based on SpaceRequest CR), not a generic one. I would structure it so it's easy to understand - something like this:

	// +optional
	SpaceRequestConfig SpaceRequestConfig `json:"spaceRequestConfig, omitempty"`
}

type SpaceRequestConfig struct {
	// +optional
	CopySaToken bool `json:"copySaToken,omitempty"`
}

if needed, we could easily extend the part by other values in the future, for example:

type SpaceRequestConfig struct {
	// +optional
	CopySaToken bool `json:"copySaToken,omitempty"`
	// +optional
	ManagerSaName string `json:"managerSaName,omitempty"`
}

another approach would be:

type SpaceRequestConfig struct {
	// +optional
	SaTokenToCopy string `json:"saTokenToCopy,omitempty"`
}

if nothing is provided, then the controller wouldn't copy anything, otherwise, it would contain the name of the SA whose token should be copied - ie. namespace-manager

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.

Let's go with the last one . Looks better.
type SpaceRequestConfig struct {
// +optional
SaTokenToCopy string json:"saTokenToCopy,omitempty"
}

@Kartikey-star
Copy link
Contributor Author

I don't see any of the generated files to be modified as part of this PR - when you run make generate then it should update zz_generated.deepcopy.go zz_generated.openapi.go (doesn't have to be both - it depends on the type of the changes)
I have named my fork as https://github.com/Kartikey-star/toolchain-api...Due to this i believe the changes in zz_generated.openapi.go were showing in a different directory which does not exist on master

@mfrancisc
Copy link
Contributor

I have named my fork as https://github.com/Kartikey-star/toolchain-api...Due to this i believe the changes in zz_generated.openapi.go were showing in a different directory which does not exist on master

@Kartikey-star yes, it's recommended to clone all the projects into this folder structure in order for the make generate target to work:

~/go/src/github.com/codeready-toolchain/api

same for the other repos host-operator|registration-service|member-operator|toolchain-e2e ....

Signed-off-by: Kartikey Mamgain <[email protected]>
@Kartikey-star
Copy link
Contributor Author

Thanks @mfrancisc , I had cloned the repos outside my GOPATH..Fixing that seems to work.

Copy link
Contributor

@MatousJobanek MatousJobanek left a comment

Choose a reason for hiding this comment

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

could you please add/fix the comments of the params and the new struct to describe the purpose of it?

SpaceRequestConfigName SpaceRequestConfig `json:"spaceRequestConfig,omitempty"`
}

type SpaceRequestConfig struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
type SpaceRequestConfig struct {
// +k8s:openapi-gen=true
type SpaceRequestConfig struct {

Signed-off-by: Kartikey Mamgain <[email protected]>
@@ -22,6 +22,17 @@ type NSTemplateTierSpec struct {
// +optional
// +mapType=atomic
SpaceRoles map[string]NSTemplateTierSpaceRole `json:"spaceRoles,omitempty"`

// Provides the name of the Service Account whose token is to be copied
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 - but I guess this comment relates more to the SaTokenToCopy field, while the struct comment could be something generic like: "SpaceRequestConfigName stores all the configuration related to the Space Request feature".

WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

same for the comment on the struct at line 31.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That seems better to me .Updated .

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 👍

Thanks for addressing my comments!

Signed-off-by: Kartikey Mamgain <[email protected]>
@Kartikey-star Kartikey-star requested a review from xcoulon October 17, 2023 06:27
Comment on lines 26 to 28
// SpaceRequestConfigName stores all the configuration related to the Space Request feature
// +optional
SpaceRequestConfigName *SpaceRequestConfig `json:"spaceRequestConfig,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is there the "Name" suffix?
It should be

Suggested change
// SpaceRequestConfigName stores all the configuration related to the Space Request feature
// +optional
SpaceRequestConfigName *SpaceRequestConfig `json:"spaceRequestConfig,omitempty"`
// SpaceRequestConfig stores all the configuration related to the Space Request feature
// +optional
SpaceRequestConfig *SpaceRequestConfig `json:"spaceRequestConfig,omitempty"`

Copy link
Contributor

@MatousJobanek MatousJobanek left a comment

Choose a reason for hiding this comment

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

I just noticed that there is .DS_Store included in the PR - what is it? could you please remove it?

@Kartikey-star
Copy link
Contributor Author

I just noticed that there is .DS_Store included in the PR - what is it? could you please remove it?

This file does exists on the main branch itself https://github.com/codeready-toolchain/api, it got altered hence committed. Shall i delete it?

Signed-off-by: Kartikey Mamgain <[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

@MatousJobanek MatousJobanek left a comment

Choose a reason for hiding this comment

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

thanks a lot for the additional changes 🙏 🥇

@Kartikey-star Kartikey-star merged commit acdc61b into codeready-toolchain:master Oct 17, 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.

4 participants