⚠️ Move `.spec.install.namespace` and `.spec.install.serviceAccount` to `.spec.namespace` and `.spec.serviceAccount` #1439

everettraven · 2024-11-08T21:31:10Z

Description

Move .spec.install.namespace and .spec.install.serviceAccount to .spec.namespace and .spec.serviceAccount, updates descriptions of those fields, and makes .spec.install optional.

Immediate follow up to #1404

Reviewer Checklist

API Go Documentation
Tests: Unit Tests (and E2E Tests, if appropriate)
Comprehensive Commit Messages
Links to related GitHub Issue(s)

netlify · 2024-11-08T21:31:23Z

✅ Deploy Preview for olmv1 ready!

Name	Link
🔨 Latest commit	`2774cf3`
🔍 Latest deploy log	https://app.netlify.com/sites/olmv1/deploys/672e9f4cea9be10008ec5e4e
😎 Deploy Preview	https://deploy-preview-1439--olmv1.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

joelanford · 2024-11-08T21:33:51Z

api/v1alpha1/clusterextension_types.go

@@ -43,6 +43,34 @@ const (

 // ClusterExtensionSpec defines the desired state of ClusterExtension
 type ClusterExtensionSpec struct {
+	// namespace is a reference to a Kubernetes namespace.
+	// This is the namespace the provided ServiceAccount must exist.


Suggested change

// This is the namespace the provided ServiceAccount must exist.

// This is the namespace in which the provided ServiceAccount must exist.

joelanford · 2024-11-08T21:36:28Z

internal/controllers/clusterextension_admission_test.go

+		errMsg        string
+	}{
+		{
+			"install specified, nothing configured",


Add field names to the struct instances?

Signed-off-by: everettraven <[email protected]>

LalatenduMohanty

/lgtm

Signed-off-by: everettraven <[email protected]>

openshift-ci · 2024-11-08T23:31:25Z

New changes are detected. LGTM label has been removed.

codecov · 2024-11-08T23:38:58Z

Codecov Report

Attention: Patch coverage is 53.33333% with 7 lines in your changes missing coverage. Please review.

Project coverage is 74.88%. Comparing base (6c2be08) to head (2774cf3).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
api/v1alpha1/zz_generated.deepcopy.go	16.66%	4 Missing and 1 partial ⚠️
internal/applier/helm.go	66.66%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1439      +/-   ##
==========================================
- Coverage   74.91%   74.88%   -0.04%     
==========================================
  Files          42       42              
  Lines        3237     3241       +4     
==========================================
+ Hits         2425     2427       +2     
- Misses        639      643       +4     
+ Partials      173      171       -2

Flag	Coverage Δ
e2e	`51.95% <53.33%> (+0.02%)`	⬆️
unit	`57.17% <0.00%> (-0.11%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

joelanford · 2024-11-09T00:15:07Z

Any docs updates necessary for this change?

everettraven · 2024-11-09T00:38:08Z

Any docs updates necessary for this change?

Ah, probably. I'll look into this on Monday.

m1kola

Looks good to me.

I see there is a question about docs - I had a look and our docs are broken already unfortunately (even getting started page). It is because previous changes were not reflected in the docs (e.g. pollInterval being renamed to pollIntervalMinutes).

I'm going to merge this PR as is and will go through the docs in the afternoon.

grokspawn · 2024-11-11T21:00:12Z

Related to #1426

There were several intentional breaking changes in the API which are now included in v0.18.0 release. Our upgrade-e2e job need to be updated to make take the changes into account. Relevant PRs: * operator-framework#1439 * operator-framework#1434 Signed-off-by: Mikalai Radchuk <[email protected]>

There were several intentional breaking changes in the API which are now included in v0.18.0 release. Our upgrade-e2e job need to be updated to make take the changes into account. Relevant PRs: * #1439 * #1434 Signed-off-by: Mikalai Radchuk <[email protected]>

There were several intentional breaking changes in the API which are now included in v0.18.0 release. This commit mostly focuses on updating the documentation to reflect API changes. This includes making sure that snippets and example outputs match the current state of the project. Relevant PRs: * operator-framework#1439 * operator-framework#1434 Together with fixing snippets & outputs this fixes some rendering issues. Signed-off-by: Mikalai Radchuk <[email protected]>

There were several intentional breaking changes in the API which are now included in v0.18.0 release. This commit mostly focuses on updating the documentation to reflect API changes. This includes making sure that snippets and example outputs match the current state of the project. Relevant PRs: * operator-framework#1439 * operator-framework#1434 Signed-off-by: Mikalai Radchuk <[email protected]>

There were several intentional breaking changes in the API which are now included in v0.18.0 release. This commit mostly focuses on updating the documentation to reflect API changes. This includes making sure that snippets and example outputs match the current state of the project. Relevant PRs: * #1439 * #1434 Signed-off-by: Mikalai Radchuk <[email protected]>

There were several intentional breaking changes in the API which are now included in v0.18.0 release. This commit mostly focuses on updating the documentation to reflect API changes. This includes making sure that snippets and example outputs match the current state of the project. Relevant PRs: * operator-framework#1439 * operator-framework#1434 Signed-off-by: Mikalai Radchuk <[email protected]>

There were several intentional breaking changes in the API which are now included in v0.18.0 release. This commit mostly focuses on updating the documentation to reflect API changes. This includes making sure that snippets and example outputs match the current state of the project. Relevant PRs: * #1439 * #1434 Signed-off-by: Mikalai Radchuk <[email protected]>

There were several intentional breaking changes in the API which are now included in v0.18.0 release. This commit mostly focuses on updating the documentation to reflect API changes. This includes making sure that snippets and example outputs match the current state of the project. Relevant PRs: * operator-framework#1439 * operator-framework#1434 Signed-off-by: Mikalai Radchuk <[email protected]>

There were several intentional breaking changes in the API which are now included in v0.18.0 release. This commit mostly focuses on updating the documentation to reflect API changes. This includes making sure that snippets and example outputs match the current state of the project. Relevant PRs: * #1439 * #1434 Signed-off-by: Mikalai Radchuk <[email protected]>

everettraven requested a review from a team as a code owner November 8, 2024 21:31

everettraven marked this pull request as draft November 8, 2024 21:31

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 8, 2024

joelanford reviewed Nov 8, 2024

View reviewed changes

everettraven added 2 commits November 8, 2024 17:42

move namespace and serviceAccount fields to spec root

7b50667

Signed-off-by: everettraven <[email protected]>

address comments, make verify

f6dd2eb

Signed-off-by: everettraven <[email protected]>

everettraven force-pushed the api/namespace-sa-root branch from 1685475 to f6dd2eb Compare November 8, 2024 22:43

everettraven marked this pull request as ready for review November 8, 2024 22:43

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 8, 2024

LalatenduMohanty previously approved these changes Nov 8, 2024

View reviewed changes

openshift-ci bot assigned LalatenduMohanty Nov 8, 2024

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 8, 2024

LalatenduMohanty enabled auto-merge November 8, 2024 22:53

fix e2e failures

2774cf3

Signed-off-by: everettraven <[email protected]>

everettraven dismissed LalatenduMohanty’s stale review via 2774cf3 November 8, 2024 23:31

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Nov 8, 2024

m1kola approved these changes Nov 11, 2024

View reviewed changes

LalatenduMohanty added this pull request to the merge queue Nov 11, 2024

Merged via the queue into operator-framework:main with commit aaa0e00 Nov 11, 2024
15 of 19 checks passed

m1kola mentioned this pull request Nov 11, 2024

📖 Fix docs: "Getting Started" and "Tutorials" #1443

Merged

7 tasks

m1kola mentioned this pull request Nov 12, 2024

🌱 Fix upgrade-e2e CI job #1445

Merged

4 tasks

m1kola mentioned this pull request Nov 14, 2024

📖 Fix docs: "How to" section #1463

Merged

4 tasks

m1kola mentioned this pull request Nov 15, 2024

📖 Fix docs: "Conceptual Guides" section #1473

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

⚠️ Move `.spec.install.namespace` and `.spec.install.serviceAccount` to `.spec.namespace` and `.spec.serviceAccount` #1439

⚠️ Move `.spec.install.namespace` and `.spec.install.serviceAccount` to `.spec.namespace` and `.spec.serviceAccount` #1439

Uh oh!

everettraven commented Nov 8, 2024

Uh oh!

netlify bot commented Nov 8, 2024 •

edited

Loading

Uh oh!

joelanford Nov 8, 2024

Uh oh!

everettraven Nov 8, 2024

Uh oh!

joelanford Nov 8, 2024

Uh oh!

everettraven Nov 8, 2024

Uh oh!

LalatenduMohanty left a comment

Uh oh!

openshift-ci bot commented Nov 8, 2024

Uh oh!

codecov bot commented Nov 8, 2024 •

edited

Loading

Uh oh!

joelanford commented Nov 9, 2024

Uh oh!

everettraven commented Nov 9, 2024

Uh oh!

m1kola left a comment

Uh oh!

Uh oh!

grokspawn commented Nov 11, 2024

Uh oh!

Uh oh!

	// This is the namespace the provided ServiceAccount must exist.
	// This is the namespace in which the provided ServiceAccount must exist.

⚠️ Move .spec.install.namespace and .spec.install.serviceAccount to .spec.namespace and .spec.serviceAccount #1439

⚠️ Move .spec.install.namespace and .spec.install.serviceAccount to .spec.namespace and .spec.serviceAccount #1439

Uh oh!

Conversation

everettraven commented Nov 8, 2024

Description

Reviewer Checklist

Uh oh!

netlify bot commented Nov 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for olmv1 ready!

Uh oh!

joelanford Nov 8, 2024

Choose a reason for hiding this comment

Uh oh!

everettraven Nov 8, 2024

Choose a reason for hiding this comment

Uh oh!

joelanford Nov 8, 2024

Choose a reason for hiding this comment

Uh oh!

everettraven Nov 8, 2024

Choose a reason for hiding this comment

Uh oh!

LalatenduMohanty left a comment

Choose a reason for hiding this comment

Uh oh!

openshift-ci bot commented Nov 8, 2024

Uh oh!

codecov bot commented Nov 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

joelanford commented Nov 9, 2024

Uh oh!

everettraven commented Nov 9, 2024

Uh oh!

m1kola left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

grokspawn commented Nov 11, 2024

Uh oh!

Uh oh!

⚠️ Move `.spec.install.namespace` and `.spec.install.serviceAccount` to `.spec.namespace` and `.spec.serviceAccount` #1439

⚠️ Move `.spec.install.namespace` and `.spec.install.serviceAccount` to `.spec.namespace` and `.spec.serviceAccount` #1439

netlify bot commented Nov 8, 2024 •

edited

Loading

codecov bot commented Nov 8, 2024 •

edited

Loading