-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
RHDEVDOCS-6317: Post GA changes for GitOps 1.16 #91491
base: gitops-docs-main
Are you sure you want to change the base?
RHDEVDOCS-6317: Post GA changes for GitOps 1.16 #91491
Conversation
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.
/lgtm
Thanks
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.
LGTM otherwise
1fe209e
to
2314de8
Compare
@Dhruv-Soni11: This pull request references RHDEVDOCS-6317 which is a valid jira issue. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
@Dhruv-Soni11: This pull request references RHDEVDOCS-6317 which is a valid jira issue. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
/label peer-review-needed |
/label peer-review-needed |
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.
Please note that updates to release notes outside of a major, minor, or patch release requires change management. Consult with your DPM or content strategist to see if this PR qualifies for an "announcement" change management email to stakeholders. Please refer to the OpenShift Docs Manual or reach out on Slack if you have questions about the CM process.
Otherwise, just a small style issue regarding API object formatting.
/label peer-review-done
/remove-label peer-review-needed
/remove-label peer-review-in-progress
@michaelryanpeter: Those labels are not set on the issue: In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
@Dhruv-Soni11: This pull request references RHDEVDOCS-6317 which is a valid jira issue. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
Thanks for the suggestion @michaelryanpeter. I have sent a Change Management email to the required stakeholders as per the process. Once I get their acks, I will send it for the merge review round. |
c72d380
to
7dc05c3
Compare
@Dhruv-Soni11: all tests passed! Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
@Dhruv-Soni11: This pull request references RHDEVDOCS-6317 which is a valid jira issue. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
CS Ack. I think change management is required primarily cause we added this: https://github.com/openshift/openshift-docs/pull/91491/files#diff-a86d57b5759e9b51c7809d425db698aa2e9aec3115aaadb163c554e6f8121265R103. |
/lgtm |
Yes @Preeticp, that is the reason I have initiated this change management request. Thanks for the acknowledgement. |
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.
LGTM
=== Deprecation of `.spec.initialRepositories` & `.spec.repositoryCredentials` fields in Argo CD | ||
|
||
* In {gitops-title} v1.16, the `.spec.initialRepositories` and `.spec.repositoryCredentials` fields in Argo CD are deprecated and will be removed in a future release. Update your configurations to remove dependencies on these fields, as they will no longer be supported. To add or modify a repository, use the Argo CD web UI or CLI. link:https://issues.redhat.com/browse/GITOPS-5961[GITOPS-5961] |
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.
As written, this sentence is incorrect (fields will not be removed, and .spec
fields are directly not part of Argo CD). This is a corrected version (@anandrkskd can correct me if wrong):
- In {gitops-title} v1.16, the
.spec.initialRepositories
and.spec.repositoryCredentials
fields in theArgoCD
CR are deprecatedand will be removed in a future release. These fields will no longer be supported by {gitops-title} and Argo CD in a future release. Update your configurations to remove dependencies on these fieldsas they will no longer be supported. To add or modify a repository, use the Argo CD web UI or CLI. link:https://issues.redhat.com/browse/GITOPS-5961[GITOPS-5961]
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.
If I understand correctly, we want to remove .spec.initialRepositories
and .spec.repositoryCredentials
field from ArogCD CR in upcoming 1.17 release, as per the discussion happened in last cabal call.
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.
@jgwest, please let me know what do you think?
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.
Both sentences make sense to me. In 1.17, even if we don't remove the fields, we will drop the functionality as upstream doesn't support it and eventually remove these fields whenever we do a API version upgrade of the ArgoCD CR in future.
What do you think about?
In {gitops-title} v1.16, the
.spec.initialRepositories
and.spec.repositoryCredentials
fields in theArgoCD
CR are deprecated and will be removed in a future release. These fields will no longer be supported by {gitops-title} in a future release. Update your configurations to remove dependencies on these fields. To add or modify a repository, use the Argo CD web UI or CLI. link:https://issues.redhat.com/browse/GITOPS-5961[GITOPS-5961]
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.
Wouldn't it be more confusing for users to have fields but not working?
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 think it is an accepted practice to keep deprecated fields around without functionality. With proper documentation/ k8s events, we can avoid confusion.
The recommended approach is to remove fields only when performing a CRD API upgrade.
LGTM - DPM |
Version(s):
GitOps 1.16
Issue:
https://issues.redhat.com/browse/RHDEVDOCS-6317
Link to docs preview:
Incorrectly rendering attribute in the Enable the creation of aggregated cluster roles section
Deprecated and removed features in GitOps 1.16 Release Notes
Note under the About RedHat OpenShift GitOps section
QE review:
Change management:
Need an ACK from Eng: [email protected] @svghadi
Need an ACK from PM: @harrietgrace
Need an ACK from Product Release Lead: @anand Francis
Need an ACK from QE: @varshab1210
Need an ACK from DPM: [email protected]
Need an ACK from CS: @Preeticp
Need an ACK from Peer reviewer: @michaelryanpeter
Need an ACK from Merge reviewer:
Additional information: