-
Notifications
You must be signed in to change notification settings - Fork 100
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
Make devsetup/cifwm_prepare not fail when rerun #304
base: main
Are you sure you want to change the base?
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.
If really, I'd rather get something checking for the directory availability, if already there, git pull
in order to ensure we're up-to-date... ?
e7d5b53
to
142078a
Compare
/lgtm |
142078a
to
682cf24
Compare
Build failed (check pipeline). For information on how to proceed, see https://review.rdoproject.org/zuul/buildset/ccc6dfdb921e476484e0f291ae6ca993 ❌ centos-9-crc-singlenode-podified-deployment FAILURE in 1h 03m 42s |
recheck |
Build failed (check pipeline). For information on how to proceed, see https://review.rdoproject.org/zuul/buildset/152db18b890449808dd92916b814d2d5 ❌ centos-9-crc-singlenode-podified-deployment FAILURE in 27m 48s |
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.
makes sense
devsetup/Makefile
Outdated
@@ -175,10 +175,13 @@ edpm_deploy_instance: ## Spin a instance on edpm node | |||
edpm_play_cleanup: ## Cleanup EDPM openstackansibleee resource | |||
-oc delete openstackansibleee deploy-external-dataplane-compute | |||
|
|||
.PHONY: cifmw_prepare | |||
cifmw_prepare: ## Clone the ci-framework repository in the ci-framework directory. That location is ignored from git. | |||
./ci-framework: |
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.
names ;)
i'd call this something more descriptive though, like ci-framework-clone or somesuch.
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.
Oh yep, i would guess the aim was to also invoke the setup part, not sure - with that (renaming...) I would prefer to wait for Cedric.
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.
maybe we can add some more "checks" in there in order to ensure we can consume the framework - for instance "is ansible present" and related? or, even, run an actual task that will create the venv and make things ready to consume?
I really added that one in the very-very early stages of the framework, just to show "hey, we're here now". We can of course evolve (actually, we must evolve ;)).
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.
So what would be the recommendation for this PR please?
Rename (but that would involve changing all the docs / local flows too)? Or add more setup steps (any specific ones to add, or I can pick any)? Or leave it be as is?
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.
thanks for discussion today lets get this merged and you can update for more functionality as needed (imo)
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
devsetup/Makefile
Outdated
@@ -175,10 +175,13 @@ edpm_deploy_instance: ## Spin a instance on edpm node | |||
edpm_play_cleanup: ## Cleanup EDPM openstackansibleee resource | |||
-oc delete openstackansibleee deploy-external-dataplane-compute | |||
|
|||
.PHONY: cifmw_prepare | |||
cifmw_prepare: ## Clone the ci-framework repository in the ci-framework directory. That location is ignored from git. | |||
./ci-framework: |
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.
thanks for discussion today lets get this merged and you can update for more functionality as needed (imo)
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: marios, queria 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 |
Currently if one tries to rerun cifwm_prepare it fails on git cloning attempt since the directory is already in place. Example of failure: > + make cifmw_prepare > git clone https://github.com/openstack-k8s-operators/ci-framework ci-framework > fatal: destination path 'ci-framework' already exists and is not an empty directory. > make: *** [Makefile:180: cifmw_prepare] Error 128 As that is not expected from make target, make it so that this target depends on the directory existing target, which itself calls the git clone for its creation.
682cf24
to
fd5f09f
Compare
New changes are detected. LGTM label has been removed. |
Build failed (check pipeline). For information on how to proceed, see https://review.rdoproject.org/zuul/buildset/685ce9b406cc4373a889399c8caec442 ❌ install-yamls-crc-podified-edpm-baremetal FAILURE in 20m 50s |
Currently if one tries to rerun cifwm_prepare it fails on git cloning attempt since the directory is already in place.
Example of failure:
As that is not expected from make target, make it so that this target depends on the directory existing target, which itself calls the git clone for its creation.