-
Notifications
You must be signed in to change notification settings - Fork 140
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 release version to status #1023
Add release version to status #1023
Conversation
controllers/datasciencecluster/datasciencecluster_controller.go
Outdated
Show resolved
Hide resolved
in the case of upgrade from 2.9.3 to 2.10.0 (assume this change gets into 2.10.0) user will see the new Release filed after upgrade, right? |
pkg/cluster/cluster_config.go
Outdated
Version version.OperatorVersion `json:"version,omitempty"` | ||
} | ||
|
||
func SetRelease(cli client.Client) (*Release, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May be it is GetRelease() since creates the structure and does not set it anywhere (and in sync with the error message).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
pkg/cluster/cluster_config.go
Outdated
Version version.OperatorVersion `json:"version,omitempty"` | ||
} | ||
|
||
func SetRelease(cli client.Client) (*Release, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I personally do not see reason for pointer (*Release) here since you anyway dereference it right after the call and optimiser should avoid extra structure copying, but it's a matter of taste, probably.
operatorReleaseVersion, err := cluster.SetRelease(r.Client) | ||
if err != nil { | ||
r.Log.Error(err, "failed to get operator release version") | ||
return ctrl.Result{}, err |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we consider it as a critical error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We probably need this to be critical error. It points to either error getting platform or CSV
@@ -257,10 +257,17 @@ func (r *DataScienceClusterReconciler) Reconcile(ctx context.Context, req ctrl.R | |||
} | |||
|
|||
// finalize reconciliation | |||
operatorReleaseVersion, err := cluster.SetRelease(r.Client) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if it would be useful to retrieve the version information before reconciling components? in case there are problems with certain components...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left minor comments, but overall I think it is fine.
@zdtsw @AjayJagan Fixed the unit tests. Can you add the lgtm again? |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: AjayJagan, etirelli, zdtsw 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:
Approvers can indicate their approval by writing |
/lgtm |
34283e7
into
opendatahub-io:incubation
* Add release version to status * Set Release version * Address comments and update manifests * Update api docs * Fix unit tests * fix linters (cherry picked from commit 34283e7)
Description
Jira Issue: https://issues.redhat.com/browse/RHOAIENG-6292
Note, this is an initialization PR i.e the changes introduce only the status field for release in DSC and DSCI for 2.10
We need a followup PR to introduce changes in upgrade logic using this version(2.11)
How Has This Been Tested?
Merge criteria: