-
Notifications
You must be signed in to change notification settings - Fork 24
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 CI manifest #341
fix CI manifest #341
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
Makefile
Outdated
@@ -316,7 +316,7 @@ else | |||
endif | |||
|
|||
.PHONY: ci-manifest-build | |||
ci-manifest-build: manifest-build ## Build CI manifest | |||
ci-manifest-build: manifest ## Build CI manifest |
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 "ci-manifest-build" runs "manifest" which itself does a push, then "ci-manifest-push" is now useless (or overlapping)?
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.
correct, that doesn't seems to be the issue
https://github.com/netobserv/network-observability-operator/actions/runs/5050617460/jobs/9061544674?pr=340
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.
but it works like this in other components:
make images
build and push manifestmake ci-manifest
overrides the manifest shortlived
Codecov Report
@@ Coverage Diff @@
## main #341 +/- ##
=======================================
Coverage 51.42% 51.42%
=======================================
Files 43 43
Lines 5186 5186
=======================================
Hits 2667 2667
Misses 2323 2323
Partials 196 196
Flags with carried forward coverage won't be shown. Click here to find out more. |
db493a6
to
06fcaf3
Compare
@@ -316,7 +316,7 @@ else | |||
endif | |||
|
|||
.PHONY: ci-manifest-build | |||
ci-manifest-build: manifest-build ## Build CI manifest | |||
ci-manifest-build: ## Build CI manifest |
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.
do we need to do the same for all other repos too ?
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.
✔️ yes !
@@ -41,8 +41,10 @@ jobs: | |||
run: IMAGE_ORG=${{ env.ORG }} IMAGE=${{ env.REGISTRY }}/${{ env.IMAGE }}:${{ env.short_sha }} make image-build | |||
- name: push images | |||
run: IMAGE_ORG=${{ env.ORG }} IMAGE=${{ env.REGISTRY }}/${{ env.IMAGE }}:${{ env.short_sha }} make image-push | |||
- name: build and push manifest | |||
run: IMAGE_ORG=${{ env.ORG }} IMAGE=${{ env.REGISTRY }}/${{ env.IMAGE }}:${{ env.short_sha }} make ci-manifest | |||
- name: build temp manifest |
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'm trying these steps locally, it fails because it looks for image tags like temp-amd64
Replaced by #342 |
Manifest image needs to be pushed to created the shortlived image