-
Notifications
You must be signed in to change notification settings - Fork 15
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
Conversation
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
Codecov ReportAll modified and coverable lines are covered by tests ✅
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
Flags with carried forward coverage won't be shown. Click here to find out more. |
@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 |
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 |
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.
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 ?
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}' |
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.
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 ?
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.
(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
)
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.
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
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 I was planning to align other repos
/approve |
[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 |
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