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 remaining Makefile issues + annoying bundle date #291

Merged
merged 2 commits into from
Mar 13, 2023

Conversation

jotak
Copy link
Member

@jotak jotak commented Mar 9, 2023

After the recent reworking on the Makefile, there were still some pending issues. E.g. we introduced a "deploy-latest" target but it did not allow to override the operator image.

This is rollbacking part of the last changes:

  • deploy-latest is removed, we can run "make deploy" as we used to do before
  • make deploy now uses again "main" images by default, as before
  • keeping the behaviour that Mohamed introduced, "make bundle" keeps using the provided IMG
  • UNLIKE before: running "make bundle" alone will NOT update the bundle so it is ready for commit; For that, a new target "make update-bundle" is introduced
  • "make bundle" does not generate the creation date as it used to do. This is now done via "make bundle-build". It means that release scripts have to be updated so they generate the creation date.

After the recent reworking on the Makefile, there were still some
pending issues. E.g. we introduced a "deploy-latest" target but it did
not allow to override the operator image.

This is rollbacking part of the last changes:
- deploy-latest is removed, we can run "make deploy" as we used to do
  before
- make deploy now uses again "main" images by default, as before
- keeping the behaviour that Mohamed introduced, "make bundle" keeps
  using the provided IMG
- UNLIKE before: running "make bundle" alone will NOT update the bundle
  so it is ready for commit; For that, a new target "make update-bundle"
is introduced
- "make bundle" does not generate the creation date as it used to do.
  This is now done via "make bundle-build". It means that RELEASE
SCRIPTS HAVE TO BE UPDATED so they generate the creation date.
@codecov
Copy link

codecov bot commented Mar 9, 2023

Codecov Report

Merging #291 (709b441) into main (2e95047) will increase coverage by 0.19%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #291      +/-   ##
==========================================
+ Coverage   47.42%   47.61%   +0.19%     
==========================================
  Files          43       43              
  Lines        4951     5015      +64     
==========================================
+ Hits         2348     2388      +40     
- Misses       2395     2416      +21     
- Partials      208      211       +3     
Flag Coverage Δ
unittests 47.61% <ø> (+0.19%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 7 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

OlivierCazade
OlivierCazade previously approved these changes Mar 9, 2023
Copy link
Contributor

@OlivierCazade OlivierCazade left a comment

Choose a reason for hiding this comment

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

LGTM

$(OCI_BIN) build -f bundle.Dockerfile -t $(BUNDLE_IMG) .
mv tmp-bundle ./bundle/manifests/netobserv-operator.clusterserviceversion.yaml
Copy link
Contributor

Choose a reason for hiding this comment

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

If the Docker build fails (line 333), then it won't move the file back to its original state.  An improvement is to put a leading - so it ignores any error and hence, the next statement will run.
-$(OCI_BIN) build -f bundle.Dockerfile -t $(BUNDLE_IMG) .

The slight drawback is that it will say the error is ignored.  It's also not a complete solution since aborting with ctrl-c will still be a problem.  The best practice is to modify the copied file instead, but I understand it may not be easily done in this situation.

Copy link
Member Author

Choose a reason for hiding this comment

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

I did what you suggested first - I think that's ok, we can live with the little drawback

Copy link
Contributor

Choose a reason for hiding this comment

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

/lgtm

@jotak
Copy link
Member Author

jotak commented Mar 13, 2023

/approve

@openshift-ci
Copy link

openshift-ci bot commented Mar 13, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jotak

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-merge-robot openshift-merge-robot merged commit 84c7fe8 into netobserv:main Mar 13, 2023
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.

None yet

4 participants