-
Notifications
You must be signed in to change notification settings - Fork 141
Pass AL_TAG to build_plugins.sh #966
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
Conversation
@@ -76,7 +74,8 @@ linux-plugins: | |||
--CLOUDWATCH_PLUGIN_CLONE_URL=${CLOUDWATCH_PLUGIN_CLONE_URL} \ | |||
--CLOUDWATCH_PLUGIN_TAG=${CLOUDWATCH_PLUGIN_TAG} \ | |||
--CLOUDWATCH_PLUGIN_BRANCH=${CLOUDWATCH_PLUGIN_BRANCH} \ | |||
--DOCKER_BUILD_FLAGS=${DOCKER_BUILD_FLAGS} | |||
--DOCKER_BUILD_FLAGS=${DOCKER_BUILD_FLAGS} \ | |||
--AL_TAG=${AL_TAG} |
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.
nit: indentation seems off
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.
Huh, whitespace mysteries, can fix
@@ -107,6 +109,12 @@ do | |||
--DOCKER_BUILD_FLAGS) | |||
if [ -n "$2" ];then PLUGIN_BUILD_ARGS="$PLUGIN_BUILD_ARGS $2";fi | |||
shift 2;; | |||
--AL_TAG) | |||
if [ -n "$2" ]; then | |||
PLUGIN_BUILD_ARGS="$PLUGIN_BUILD_ARGS --build-arg AL_TAG=$2" |
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.
Where/how is this eventually used?
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.
The -AL_TAG override will be used when calling make-release/make-debug which currently assigns a default 2 in the makefile. For AL2023, we will override that default to AL2023 and it will flow down to all make targets and get passed into this script --AL_TAG=${AL_TAG}
== --AL_TAG=2023
.
I tried to cut-up the larger PR for all of the changes to make it easier to focus and not get lost in details
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 see. But why do the plugin builds need to be AL aware? We're doing go build
for each of the Go plugins we bake in the image right?
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 suppose that is true, they do not need to be aware. We go build each plugin and then source that into the final image
Synced offline, and while it would be nice to have AL_TAG controlling the AL version for plugin builds. It is not required and should not effect the final runtime image. There is already a pull request related to removing the gimme/go approach in favor of an image using specific go version - #875, which is the better long term approach |
Summary
Fix issue that AL_TAG is not passed to build_plugins.sh script. This prevents overriding the AL_TAG.
Minor re-org for linux-plugins/windows-plugins export definition
Testing
Tested by building local plugins and validating that AL_TAG override:
make debug
succeeded: yesInteg tests succeeded: yes
New tests cover the changes: no
Description for the changelog
Pass AL_TAG to build_plugins.sh
Licensing
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.