Skip to content

ROX-15327: Remove Circle CI references and simplify tagging#5080

Merged
gavin-stackrox merged 12 commits intomasterfrom
gavin/ROX-15327/simpler-tag-and-less-circle
Mar 9, 2023
Merged

ROX-15327: Remove Circle CI references and simplify tagging#5080
gavin-stackrox merged 12 commits intomasterfrom
gavin/ROX-15327/simpler-tag-and-less-circle

Conversation

@gavin-stackrox
Copy link
Copy Markdown
Contributor

@gavin-stackrox gavin-stackrox commented Mar 3, 2023

Description

Less is more! And tag determination was overly complicated.

Checklist

  • Investigated and inspected CI test results

Testing Performed

  • Regular CI - unrelated flake in postgres upgrade
  • Release simulation (if possible)

@gavin-stackrox gavin-stackrox requested a review from a team as a code owner March 3, 2023 02:41
@gavin-stackrox gavin-stackrox requested review from davdhacs, janisz and msugakov and removed request for a team March 3, 2023 02:47
@ghost
Copy link
Copy Markdown

ghost commented Mar 3, 2023

Images are ready for the commit at 4a7a964.

To use with deploy scripts, first export MAIN_IMAGE_TAG=3.74.x-313-g4a7a9640ad.

Comment thread Makefile Outdated
Comment thread scripts/ci/lib.sh
Comment thread make/env.mk
@msugakov
Copy link
Copy Markdown
Contributor

msugakov commented Mar 3, 2023

Note that CIRCLE_TAG renaming should be mirrored in the downstream. I can help with that.

@gavin-stackrox gavin-stackrox requested a review from msugakov March 3, 2023 16:19
Comment thread scripts/reference/build-quay-operator-bundles.sh Outdated
Comment thread scripts/reference/build-quay-operator-bundles.sh Outdated
Comment thread Makefile Outdated
Comment on lines 21 to 23
ifeq ($(BUILD_TAG),)
BUILD_TAG=$(shell git describe --tags --abbrev=10 --dirty --long --exclude '*-nightly-*')
endif
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

After looking at scripts/check-debugger.sh, I remember why there was a difference between BUILD_TAG (former CIRCLE_TAG) and just TAG (that was present in this Makefile before edits) in the context of Makefile.

  • BUILD_TAG/CIRCLE_TAG meant a tag provided externally, i.e. from CI when we need to build something like 3.74.1-rc.1.
  • TAG held the current value of the tag.
    • If BUILD_TAG/CIRCLE_TAG was provided, TAG simply got its value.
    • Otherwise, TAG held a result of git describe, i.e. some 3.74.x-266-gb6315d348b thingy.

To me this explains the existence of that TAG= trick in make/env.mk (intent, but not a reasoning).

Without such separation, we'll not be able to differentiate tagged (i.e. release, RC, nightly) and untagged builds. It seems to me that scripts/check-debugger.sh is the only place that does that.
Do you know other places?
How broken is the debugger support that we can leave check-debugger.sh in incorrect state?

Copy link
Copy Markdown
Contributor

@msugakov msugakov Mar 3, 2023

Choose a reason for hiding this comment

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

It seems to me that scripts/check-debugger.sh is the only place that does that.
Do you know other places?

Here's a deeper example. Our Makefile calls into itself, e.g. in case of main-build-dockerized target

stackrox/Makefile

Lines 404 to 408 in d8f6253

.PHONY: main-build-dockerized
main-build-dockerized: main-builder-image
@echo "+ $@"
docker run $(DOCKER_USER) -i -e RACE -e CI -e BUILD_TAG -e GOTAGS -e DEBUG_BUILD --rm $(GOPATH_WD_OVERRIDES) $(LOCAL_VOLUME_ARGS) $(BUILD_IMAGE) make main-build-nodeps

The fact that BUILD_TAG environment variable is provided highlights a nice distinction that:

  • Every environment variable will be visible as makefile variable with the same name. BUILD_TAG=blah make ... will do $(BUILD_TAG) equal to blah during the Makefile execution.
  • However, it does not work the other way. Makefile variable does not become an environment variable with the same name. I.e. when ${BUILD_TAG} is not defined in the environment, main-build-dockerized propagates no BUILD_TAG value into docker run ... -e BUILD_TAG ... context.

This means, main-build-dockerized should still work correctly w.r.t. actual GOTAGS=release (or not) in the resulting binaries, but the part why it works correctly is a bit too subtle.

WDYT, should we keep things the way they are now or should we restore BUILD_TAG-TAG duality in the Makefile for better explicitness?

Looking at it from another angle, the "BUILD_TAG-TAG duality" was to differentiate when we should and shouldn't apply GOTAGS=release during Go binaries build. Maybe that's the place that should be changed to make rules more explicit and straightforward?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

WDYT, should we keep things the way they are now or should we restore BUILD_TAG-TAG duality in the Makefile for better explicitness?

I think I will put it back the way it was i.e. TAG in the make environment.

@gavin-stackrox gavin-stackrox force-pushed the gavin/ROX-15327/simpler-tag-and-less-circle branch from 36a9eed to e9136e6 Compare March 7, 2023 22:48
@gavin-stackrox gavin-stackrox requested a review from msugakov March 8, 2023 02:45
@openshift-ci
Copy link
Copy Markdown

openshift-ci bot commented Mar 8, 2023

@gavin-stackrox: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/gke-postgres-upgrade-tests 4a7a964 link false /test gke-postgres-upgrade-tests
ci/prow/gke-postgres-qa-e2e-tests 4a7a964 link false /test gke-postgres-qa-e2e-tests

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@gavin-stackrox gavin-stackrox merged commit 64973fa into master Mar 9, 2023
@gavin-stackrox gavin-stackrox deleted the gavin/ROX-15327/simpler-tag-and-less-circle branch March 9, 2023 17:01
janisz added a commit to stackrox/jenkins-plugin that referenced this pull request May 15, 2023
Reflect changes made in release scripts in

- stackrox/stackrox#5080
janisz added a commit to stackrox/jenkins-plugin that referenced this pull request May 16, 2023
Reflect changes made in release scripts in

- stackrox/stackrox#5080
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants