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

build: add gen-csv files in downstream repo #581

Merged
merged 3 commits into from
Mar 20, 2024

Conversation

subhamkrai
Copy link

@subhamkrai subhamkrai commented Feb 28, 2024

build: add gen-csv files in downstream repo

This commit add the csv related changes to downstream and
later will be removed from upstream also renaming make 'csv'
to 'gen-csv' and also adds validation check in ci to verify
right files are modified.

build: add csv envs to rook deploy envs

moving all the default envs which were part of
ocs-op rook csv to operator deployment.

build: these are auto-generated files

This commit contains auto-generated files
that are added/modified by running 'make gen-csv'.

Checklist:

  • Commit Message Formatting: Commit titles and messages follow guidelines in the developer guide.
  • Reviewed the developer guide on Submitting a Pull Request
  • Pending release notes updated with breaking and/or notable changes for the next minor release.
  • Documentation has been updated, if necessary.
  • Unit tests have been added, if necessary.
  • Integration tests have been added, if necessary.

@subhamkrai
Copy link
Author

@travisn @Nikhil-Ladha PTAL

@df-build-team df-build-team requested a review from a team February 28, 2024 08:07
@Nikhil-Ladha
Copy link
Member

cc @iamniting

@subhamkrai subhamkrai force-pushed the add-csv-gen-script branch 4 times, most recently from a644b3e to 67d3641 Compare March 5, 2024 11:39
@parth-gr
Copy link
Member

parth-gr commented Mar 5, 2024

I think we need to remove the csv in upstream, otherwise if this is merged and then upstream remove csv pr is merged then branch sync would have a problem.

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 5, 2024
@BlaineEXE
Copy link

I am also somewhat concerned that this code is special for downstream and that it may be erased when github.com/rook/rook@master is resynced here, or to release-* branches. I certainly may also be misunderstanding something here.

@subhamkrai
Copy link
Author

I am also somewhat concerned that this code is special for downstream and that it may be erased when github.com/rook/rook@master is resynced here, or to release-* branches. I certainly may also be misunderstanding something here.

If we just skip one upstream commit that removes the csv related file, downstream should be good then I guess

@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 6, 2024
@Nikhil-Ladha
Copy link
Member

I am also somewhat concerned that this code is special for downstream and that it may be erased when github.com/rook/rook@master is resynced here, or to release-* branches. I certainly may also be misunderstanding something here.

If we just skip one upstream commit that removes the csv related file, downstream should be good then I guess

We don't have to skip anything as such, once we are set to merge these changes at that time we should first delete the changes in upstream, sync those changes to downstream, rebase this PR and then merge. Then, everything should be good and we won't be facing any challenges.

And, later syncs won't be a problem because we are just bring commits from upstream to downstream, that would continue as usual.

@subhamkrai
Copy link
Author

I am also somewhat concerned that this code is special for downstream and that it may be erased when github.com/rook/rook@master is resynced here, or to release-* branches. I certainly may also be misunderstanding something here.

If we just skip one upstream commit that removes the csv related file, downstream should be good then I guess

We don't have to skip anything as such, once we are set to merge these changes at that time we should first delete the changes in upstream, sync those changes to downstream, rebase this PR and then merge. Then, everything should be good and we won't be facing any challenges.

And, later syncs won't be a problem because we are just bring commits from upstream to downstream, that would continue as usual.

IIRC, once we remove the csv generation files from upstream and sync that to downstream those file will be removed in downstream also and the csv generation will fail. But let's sync on this @Nikhil-Ladha

@subhamkrai subhamkrai force-pushed the add-csv-gen-script branch 8 times, most recently from 9b04a3e to 9568b1a Compare March 6, 2024 12:08
@subhamkrai
Copy link
Author

For me locally build.all is passing and I don't have mac to test the mac failure.

@travisn @BlaineEXE could anyone of you help in mac CI error?

@BlaineEXE
Copy link

Looking at the CI error for macos, I think the build itself is succeeding properly. But in the "validate build" step afterward, the git status --porcelain output afterwards is showing that the build created a diff between what is checked into the code.

The git diff suggests that the status wants to remove all of the CRDs, judging by the - chars along the lefthand side of the diff.

+ git diff
-apiVersion: apiextensions.k8s.io/v1
-kind: CustomResourceDefinition
-metadata:
-  annotations:
-    controller-gen.kubebuilder.io/version: v0.11.3
-  creationTimestamp: null
-  name: cephblockpoolradosnamespaces.ceph.rook.io
-spec:
-  group: ceph.rook.io
-  names:
-    kind: CephBlockPoolRadosNamespace
-    listKind: CephBlockPoolRadosNamespaceList
-    plural: cephblockpoolradosnamespaces
-    singular: cephblockpoolradosnamespace
-  scope: Namespaced
# etc...

@subhamkrai subhamkrai force-pushed the add-csv-gen-script branch 3 times, most recently from 7761477 to cdded69 Compare March 8, 2024 09:57
@subhamkrai
Copy link
Author

@subhamkrai in last offline discussion we suggested to have a separate readme.md that only talks about downstream specific changes and avoid merge conflicts. Currently this change and change for SVG is added #589 So with this PR have a plan to initiate that
cc @travisn @iamniting

I would highly suggest doing so, so that we don't miss any commits in the future branches.

I will prefer to do it in separate PR
since I'm planning to get this PR merged EOD and docs PRs generally have more reviews before they get merged

@parth-gr
Copy link
Member

I will prefer to do it in separate PR since I'm planning to get this PR merged EOD and docs PRs generally have more reviews before they get merged

I don't have any problem with separate PR, but keep that item in mind as we cant track it separately anywhere.

@subhamkrai subhamkrai force-pushed the add-csv-gen-script branch 3 times, most recently from 818482a to 9d54419 Compare March 15, 2024 14:32
@subhamkrai subhamkrai force-pushed the add-csv-gen-script branch 5 times, most recently from cb3d7f1 to 5e27346 Compare March 18, 2024 08:51
@subhamkrai
Copy link
Author

@iamniting PTAL

email: ocs-support@redhat.com
maturity: alpha
provider:
name: Provider Name
Copy link
Member

Choose a reason for hiding this comment

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

Can you update this as well?

Copy link

openshift-ci bot commented Mar 18, 2024

@iamniting: changing LGTM is restricted to collaborators

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/test-infra repository.

@iamniting
Copy link
Member

CSV changes looks good to me

since we don't need the csv generation to be part of
our build image let's remove that. And add a check in CI
to validate modified csv files.

Signed-off-by: subhamkrai <srai@redhat.com>
moving all the default envs which were part of
ocs-op rook csv to operator deployment.

Signed-off-by: subhamkrai <srai@redhat.com>
these are auto-generated csv changes

Signed-off-by: subhamkrai <srai@redhat.com>
@sp98
Copy link

sp98 commented Mar 20, 2024

/approve

Copy link

openshift-ci bot commented Mar 20, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: sp98, subhamkrai

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

Needs approval from an approver in each of these files:

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

@sp98 sp98 merged commit ee3a050 into red-hat-storage:master Mar 20, 2024
48 of 49 checks passed
@subhamkrai subhamkrai deleted the add-csv-gen-script branch March 20, 2024 10:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants