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

fix: Change project domain #96

Merged

Conversation

mateusoliveira43
Copy link
Contributor

@mateusoliveira43 mateusoliveira43 commented Oct 2, 2024

Why the changes were made

Change project domain for easier webhook integration with OADP.

Change were made following kubebuilder upgrade documentation.

Blocked by openshift/oadp-operator#1589

How to test the changes made

Check all places where changed (grep -Inr "nac.oadp.openshift.io" .)

Run the project following development documentation and check if nothing broke with the changes.

Copy link

openshift-ci bot commented Oct 2, 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

@openshift-ci openshift-ci bot added the approved label Oct 2, 2024
Copy link
Contributor Author

Choose a reason for hiding this comment

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

will remove prior to merge, just to show the diff file used

Copy link
Collaborator

Choose a reason for hiding this comment

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

still in the latest force push, need to remove that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Member

@shubham-pampattiwar shubham-pampattiwar left a comment

Choose a reason for hiding this comment

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

Please rebase this PR and also create a corresponding OADP operator PR simultaneously.

@shawn-hurley
Copy link

Why are we moving the group out of nac to just oadp?

@mateusoliveira43
Copy link
Contributor Author

@shawn-hurley , @mpryc first implementations with webhook in OADP side suggested would be easier if NAC objects have same group as OADP objects

@shubham-pampattiwar waiting to see if we will need this for that implementation. If not, I will close this one without merging

@shubham-pampattiwar
Copy link
Member

yeah we need this. Basically:

  • OADP Operator is responsible for installing the NAC CRDs, using oadp as a group makes it clear that the CRD/resource is part of the OADP ecosystem.
  • Another thing would be that oadp as a group fir NAC CRDs keeps the CRDs logically consistent and simplifies grouping for RBAC policies, docs and user understanding.

Signed-off-by: Mateus Oliveira <[email protected]>
change domain in docs

Signed-off-by: Mateus Oliveira <[email protected]>
doc change

Signed-off-by: Mateus Oliveira <[email protected]>
remove diff

Signed-off-by: Mateus Oliveira <[email protected]>
@mateusoliveira43 mateusoliveira43 marked this pull request as ready for review November 13, 2024 20:20
@openshift-ci openshift-ci bot requested review from mpryc and sseago November 13, 2024 20:20
@mateusoliveira43
Copy link
Contributor Author

/hold

@mpryc
Copy link
Collaborator

mpryc commented Nov 13, 2024

/lgtm

Copy link

openshift-ci bot commented Nov 13, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mateusoliveira43, mpryc, shubham-pampattiwar

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 [mateusoliveira43,mpryc,shubham-pampattiwar]

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

@mpryc
Copy link
Collaborator

mpryc commented Nov 14, 2024

/retry oadp-compatibility-check

@mpryc
Copy link
Collaborator

mpryc commented Nov 14, 2024

/unhold

@openshift-merge-bot openshift-merge-bot bot merged commit 697ab8f into migtools:master Nov 14, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants