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 feature toggle annotations to Space & NSTemplateSet #1048

Merged
merged 20 commits into from
Jul 1, 2024

Conversation

alexeykazakov
Copy link
Contributor

@alexeykazakov alexeykazakov commented Jun 22, 2024

Part of https://issues.redhat.com/browse/SANDBOX-653

This PR introduces feature lottery drawing for Spaces. If there is features defined in the Tier section of the Configuration then the corresponding annotation added for all winning features to the Space during its creation. This annotation is also propagated to NSTemplateSet.
See more details here: https://docs.google.com/document/d/1YWaR94JTrVBBVoMSjgIb8jb2FkqKiT1myVib9Qv0BzU/edit#bookmark=id.k3fvz630dsa6

Requires codeready-toolchain/toolchain-common#409

The member operator part is here: codeready-toolchain/member-operator#580
This PR and member operator PR are related but independent.

There is no e2e tests yet. We will think about adding some later when other pieces of the entire flow are addressed in following PRs.

Copy link

openshift-ci bot commented Jun 22, 2024

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@@ -42,228 +42,259 @@ func TestCreateSpace(t *testing.T) {
require.NoError(t, err)
base1nsTier := tiertest.Base1nsTier(t, tiertest.CurrentBase1nsTemplates)
t.Run("success", func(t *testing.T) {
// given
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems that there are a lot of changes in this test but it's actually almost the same test but wrapped with a table for two usecases:

  • with a feature toggle annotation in the Space
  • without any

Comment on lines +61 to 64
spaceOptions := []spacetest.Option{spacetest.WithSpecTargetCluster("member-1")}
if testRun.featureToggles != "" {
spaceOptions = append(spaceOptions, spacetest.WithAnnotation(toolchainv1alpha1.FeatureToggleNameAnnotationKey, testRun.featureToggles))
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the first change in this test: Adding the feature annotation to the Space depending on the specific test from the table.

Comment on lines +93 to +99
if testRun.featureToggles != "" {
nsTmplSetAssertion = nsTmplSetAssertion.
HasAnnotationWithValue(toolchainv1alpha1.FeatureToggleNameAnnotationKey, testRun.featureToggles)
} else {
nsTmplSetAssertion = nsTmplSetAssertion.
DoesNotHaveAnnotation(toolchainv1alpha1.FeatureToggleNameAnnotationKey)
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the second change. Check the NsTemplateSet feature annotation if it's present in the Space.
The rest of this test is not changed and just formatted to be moved to the table test wrapper.

Copy link
Contributor

@xcoulon xcoulon 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 overall, just comments about the var names (naming is hard! 😅)

pkg/space/space.go Show resolved Hide resolved
pkg/space/space.go Outdated Show resolved Hide resolved
Copy link
Contributor

@filariow filariow left a comment

Choose a reason for hiding this comment

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

largely LGTM. Just some ideas for enhancing tests

pkg/space/space.go Outdated Show resolved Hide resolved
pkg/space/space.go Outdated Show resolved Hide resolved
pkg/space/space.go Outdated Show resolved Hide resolved
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.

Great job 👏

I have only few minor comments.

controllers/usersignup/usersignup_controller_test.go Outdated Show resolved Hide resolved
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.

Great Job 🚀

Looks good to me!

controllers/toolchainconfig/configuration.go Outdated Show resolved Hide resolved
Copy link
Contributor

@rajivnathan rajivnathan left a comment

Choose a reason for hiding this comment

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

Looks great 👍

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.

Just one comment regarding the unit test; otherwise, it looks good 🚀

controllers/space/space_controller_test.go Outdated Show resolved Hide resolved
Copy link

openshift-ci bot commented Jul 1, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alexeykazakov, MatousJobanek, mfrancisc, rajivnathan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [MatousJobanek,alexeykazakov,mfrancisc,rajivnathan]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Copy link

codecov bot commented Jul 1, 2024

Codecov Report

Attention: Patch coverage is 93.02326% with 3 lines in your changes missing coverage. Please review.

Project coverage is 84.91%. Comparing base (43bf61e) to head (f825f55).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1048      +/-   ##
==========================================
+ Coverage   84.83%   84.91%   +0.08%     
==========================================
  Files          55       55              
  Lines        4972     5012      +40     
==========================================
+ Hits         4218     4256      +38     
- Misses        581      583       +2     
  Partials      173      173              
Files Coverage Δ
controllers/space/space_controller.go 86.88% <100.00%> (+0.42%) ⬆️
controllers/usersignup/usersignup_controller.go 76.01% <100.00%> (ø)
pkg/space/space.go 96.29% <100.00%> (+1.29%) ⬆️
controllers/toolchainconfig/configuration.go 91.42% <75.00%> (-1.21%) ⬇️

... and 1 file with indirect coverage changes

@alexeykazakov alexeykazakov merged commit a5e0448 into codeready-toolchain:master Jul 1, 2024
11 of 12 checks passed
@alexeykazakov alexeykazakov deleted the osl-host branch July 1, 2024 20:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants