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

Distinguish "clean builds" and dev builds #619

Merged
merged 1 commit into from
Oct 29, 2024
Merged

Conversation

jotak
Copy link
Member

@jotak jotak commented Oct 10, 2024

It will save time during rebuilds for the developers, since the current time isn't passed to the go build anymore, which was previously forcing to refresh the docker layers

It will save time during rebuilds for the developers, since the current
time isn't passed to the go build anymore, which was previously forcing
to refresh the docker layers
@jotak jotak added no-qe This PR doesn't necessitate QE approval no-doc This PR doesn't require documentation change on the NetObserv operator labels Oct 10, 2024
Copy link

codecov bot commented Oct 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 55.99%. Comparing base (ea2650d) to head (c064339).
Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #619      +/-   ##
==========================================
- Coverage   56.00%   55.99%   -0.01%     
==========================================
  Files         193      193              
  Lines        9482     9487       +5     
  Branches     1214     1214              
==========================================
+ Hits         5310     5312       +2     
- Misses       3795     3798       +3     
  Partials      377      377              
Flag Coverage Δ
uitests 57.70% <ø> (ø)
unittests 51.30% <ø> (-0.03%) ⬇️

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

see 1 file with indirect coverage changes

@jotak
Copy link
Member Author

jotak commented Oct 11, 2024

@jpinsonneau this PR should make dev rebuilds faster when we don't change the backend .. currently if we change only frontend and rebuild, unless it's within a minute, the BUILD_DATE changes, which is propagated into the go build command, forcing a full rebuild. With this change, BUILD_DATE will be ignored unless it's built from CI
(As a TODO , we need to check with @OlivierCazade how to make something similar with konflux)

Comment on lines +36 to +42
OCI_BUILD_OPTS ?=

ifneq ($(CLEAN_BUILD),)
BUILD_DATE := $(shell date +%Y-%m-%d\ %H:%M)
BUILD_SHA := $(shell git rev-parse --short HEAD)
LDFLAGS ?= -X 'main.buildVersion=${VERSION}-${BUILD_SHA}' -X 'main.buildDate=${BUILD_DATE}'
endif
Copy link
Contributor

Choose a reason for hiding this comment

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

In the end, only hours / minutes in the build date are forcing docker to regenerate the whole image.

Why not having just the date for local builds and full date + time on clean builds ?

Suggested change
OCI_BUILD_OPTS ?=
ifneq ($(CLEAN_BUILD),)
BUILD_DATE := $(shell date +%Y-%m-%d\ %H:%M)
BUILD_SHA := $(shell git rev-parse --short HEAD)
LDFLAGS ?= -X 'main.buildVersion=${VERSION}-${BUILD_SHA}' -X 'main.buildDate=${BUILD_DATE}'
endif
OCI_BUILD_OPTS ?=
BUILD_DATE := $(shell date +%Y-%m-%d\)
ifneq ($(CLEAN_BUILD),)
BUILD_DATE = $(shell date +%Y-%m-%d\ %H:%M)
endif
BUILD_SHA := $(shell git rev-parse --short HEAD)
BUILD_VERSION := $(TAG:v%=%)
ifneq ($(COMMIT), $(TAG_COMMIT))
BUILD_VERSION := $(BUILD_VERSION)-$(BUILD_SHA)
endif
ifneq ($(shell git status --porcelain),)
BUILD_VERSION := $(BUILD_VERSION)-dirty
endif
LDFLAGS := -X 'main.buildVersion=${BUILD_VERSION}' -X 'main.buildDate=${BUILD_DATE}'

Copy link
Member Author

Choose a reason for hiding this comment

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

this doesn't work or I need to reintroduce everything that I removed for cleaning :-) (COMMIT, TAG_COMMIT etc.)
my point was also to simplify this part ... is it really useful ?

Copy link
Member Author

@jotak jotak Oct 17, 2024

Choose a reason for hiding this comment

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

(at the end of the day, do we really need all this stuff for dev builds? my goal was to simplify all of that in dev builds, and keep that only in CI builds

if you tell me you it's useful to you, no problem I can bring it back
)

Copy link
Contributor

@jpinsonneau jpinsonneau Oct 17, 2024

Choose a reason for hiding this comment

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

For me it's only useful for releases so I'm fine skipping these for dev builds if you want to
These are only displayed in the query summary and logs.

FYI these info were coming from FLP Makefile https://github.com/netobserv/flowlogs-pipeline/blob/main/Makefile#L17-L28 so we may need to align other repos

Copy link
Member Author

Choose a reason for hiding this comment

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

yes I was planning to align other repos

@jotak
Copy link
Member Author

jotak commented Oct 17, 2024

/approve

Copy link

openshift-ci bot commented Oct 17, 2024

[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

@jotak jotak merged commit 4f95f98 into netobserv:main Oct 29, 2024
10 of 12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved lgtm no-doc This PR doesn't require documentation change on the NetObserv operator no-qe This PR doesn't necessitate QE approval
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants