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: Remove and relocate files #1246

Conversation

mateusoliveira43
Copy link
Contributor

Remove unused files and relocate files that were alone in folders.

@openshift-ci 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 30, 2023
Copy link

openshift-ci bot commented Nov 30, 2023

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately we do not have GitHub actions in this repo, so these files are not used

Except from the link checker job, I think the rest is already done by PROW

Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't that still relevant for people (like doc's team) which would want to setup such workflows on the forked repos? openshift org doesn't allow GitHub Actions, however on a forked oadp-operator we can have them.

Copy link
Member

Choose a reason for hiding this comment

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

I have in the past setup codecov workflows to run in forks. Have thrown ideas in the ring for having GHActions do code formatting in forks which would indirectly update PRs.. but people didn't like the idea of having code updated without their knowledge.

Not just docs.

.travis.yml Outdated Show resolved Hide resolved
Makefile Outdated
@@ -318,9 +318,6 @@ bundle: manifests kustomize operator-sdk ## Generate bundle manifests and metada
cd config/manager && GOFLAGS="-mod=mod" $(KUSTOMIZE) edit set image controller=$(IMG)
GOFLAGS="-mod=mod" $(KUSTOMIZE) build config/manifests | GOFLAGS="-mod=mod" $(OPERATOR_SDK) generate bundle -q --extra-service-accounts "velero" --overwrite --version $(VERSION) $(BUNDLE_METADATA_OPTS)
@make nullables
# Copy updated bundle.Dockerfile to CI's Dockerfile.bundle
# TODO: update CI to use generated one
cp bundle.Dockerfile build/Dockerfile.bundle
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@shubham-pampattiwar Can you confirm this is not needed? (we could also remove the doc file I modified)

Copy link
Contributor

Choose a reason for hiding this comment

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

Usually most of that Makefile is generated by operator-sdk, did you experiment with creating new one or upgrading to the newer version of operator-sdk and re-generating Makefile?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OWNERS_ALIASES Outdated
Copy link
Contributor Author

Choose a reason for hiding this comment

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

To use this, we would need to cite the team name in OWNERS
Reference https://github.com/kubernetes/community/blob/master/contributors/guide/owners.md

@@ -66,7 +66,7 @@ The new velero `DataUpload` and `DataDownload` will be added.

#### OADP container images

References to `velero-plugin-for-vsm` and `volume-snapshot-mover` images need to be removed from `config/manager/manager.yaml`, golang code, and `deploy/disconnected-prep.sh`.
References to `velero-plugin-for-vsm` and `volume-snapshot-mover` images need to be removed from `config/manager/manager.yaml`, golang code, and `scripts/disconnected-prep.sh`.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe would also need changes in prod docs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This would also need PROW changes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

gave up in the idea on this PR, for simplicity, but could be a valid change in the future

Copy link
Contributor Author

Choose a reason for hiding this comment

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

would also want to move docs/examples/datamover_resources.sh, docs/scripts/dc-post-restore.sh files and blogs/tekton-oadp-nonadmin/, must-gather/ folders inside this new folder

and move blogs/ folder inside docs/ folder

Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this file generated by operator-sdk operator-sdk generate bundle ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no, operator-sdk generates bundle.Dockerfile in the project root. This one is copy of it

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe downstream build requires that? Please double check with release engineering. And if needed maybe a symlink instead of raw copy?

Copy link
Member

Choose a reason for hiding this comment

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

No the build/....Dockerfiles were mostly for CI/prow related mods to run e2e/unit tests.

@mateusoliveira43 mateusoliveira43 force-pushed the fix/remove-realocate-files branch from 926e270 to 6ce23e0 Compare December 28, 2023 14:05
@mateusoliveira43 mateusoliveira43 marked this pull request as ready for review December 28, 2023 15:34
@openshift-ci 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 Dec 28, 2023
@mateusoliveira43
Copy link
Contributor Author

/retest

@mateusoliveira43 mateusoliveira43 force-pushed the fix/remove-realocate-files branch from f8691c8 to 0aa055c Compare December 29, 2023 16:48
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

lets revert this change if CI uses this file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

reverted. The details are in the issue, so it can be done later in another PR

@mateusoliveira43
Copy link
Contributor Author

/hold

Discuss change in CI files (that would also need changes in all tested branches)

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 29, 2023
@mateusoliveira43
Copy link
Contributor Author

/unhold

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 16, 2024
@mateusoliveira43 mateusoliveira43 force-pushed the fix/remove-realocate-files branch from 1441531 to 827d576 Compare March 8, 2024 17:53
@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 8, 2024
Copy link
Contributor

@weshayutin weshayutin left a comment

Choose a reason for hiding this comment

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

/lgtm

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

openshift-ci bot commented Mar 8, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mateusoliveira43, weshayutin

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD 355cf0f and 2 for PR HEAD 827d576 in total

@mateusoliveira43
Copy link
Contributor Author

/override ci/prow/4.13-e2e-test-aws
/override ci/prow/4.14-e2e-test-azure
both were successful restore but application not working after

Copy link

openshift-ci bot commented Mar 11, 2024

@mateusoliveira43: Overrode contexts on behalf of mateusoliveira43: ci/prow/4.13-e2e-test-aws, ci/prow/4.14-e2e-test-azure

In response to this:

/override ci/prow/4.13-e2e-test-aws
/override ci/prow/4.14-e2e-test-azure
both were successful restore but application not working after

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.

Copy link

openshift-ci bot commented Mar 11, 2024

@mateusoliveira43: 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/test-infra repository. I understand the commands that are listed here.

@openshift-merge-bot openshift-merge-bot bot merged commit 7cf1ba9 into openshift:master Mar 11, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants