-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Build OSS branch for deploying to Cloud env #11474
Conversation
de06836
to
8d5c1a2
Compare
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.
VERY COOL!
This action will be called by the Cloud github workflow if an OSS branch input is provided. See the Cloud PR that I'll be putting up soon.
I'm excited to this part!
I think it is worth making sure we have clear docs for this. You could likely re-purpose a lot of what you already have in the PR description.
Only putting comment because I didn't have time to review the github actions closely. The approach seems very good to me.
airbyte-bootloader/Dockerfile
Outdated
@@ -1,10 +1,14 @@ | |||
ARG JDK_VERSION=17.0.1 | |||
FROM openjdk:${JDK_VERSION}-slim | |||
|
|||
ARG VERSION=0.35.60-alpha |
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.
why do we need the arg at all? (not a leading question, i genuinely am unsure of the purpose it serves. would it make sense to always pull the version as an arg that must be passed when we build?)
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 think we could definitely go that route if we wanted to- the approach I took here with the defaulting behavior was to preserve existing behavior/dev workflows as much as possible. I wanted things to 'just work' if you run SUB_BUILD=PLATFORM ./gradle build
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.
legit. okay. i think my head is gravitating to a separate refactor. seems like we should be able to have SUB_BUILD=PLATFORM ./gradle build
"just work" and have gradle be in charge of passing the version in without having to hard code it in the dockerfile.
that said, we should not block on this!
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.
Totally agreed!
name: "Build and Push Branch" | ||
description: "Build jars and docker images tagged for a particular branch. Primarily used for running OSS branch code in Cloud." | ||
inputs: | ||
branch_version_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.
is there a way to auto generate this? if we're just doing oss-branch-<commit hash>
could the action just generate it itself?
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 Cloud workflow generates this based on the input branch name, and then passes it in to the OSS action - I needed Cloud to be in charge of generating this because the same tag/version is used elsewhere in the cloud workflow, and the Cloud workflow is what initially receives the branch name input
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.
fair. you can make it optional fwiw. so if it is not set it just generates oss-branch-<commit-hash>
but if it is set it uses what is passed in. probably not all that important though since it sounds like the only reason we would ever call this action is from the GH action in airbyte-cloud
@@ -133,7 +134,7 @@ public boolean checkOnlyPatchVersionIsUpdatedComparedTo(final AirbyteVersion ano | |||
} | |||
|
|||
public boolean isDev() { | |||
return version.equals(DEV_VERSION); | |||
return version.equals(DEV_VERSION) || version.contains(OSS_BRANCH_VERSION_PREFIX); |
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.
instead of oss-branch-<whatever>
would it makes sense to just do dev-<whatever
? i haven't thought through the tradeoffs here. you're adding a new magic word. if it has a significantly different meaning than dev it is worth having the name be separate. if they are the same conceptually then maybe repurposing dev to be either the full branch version or just a prefix seems more intuitive?
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 think that's a good idea - we'd still need to change the version.equals(DEV_VERSION)
to version.contains(DEV_PREFIX)
but that feels better to me than a whole other magic word, as you pointed out.
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.
agreed.
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.
Reviewed the github action. Looks good to me. One additional comment on sharing logic in the action, but not necessary to block as long as we create a follow up issue.
- name: Build | ||
run: VERSION=${{ inputs.branch_version_tag }} SUB_BUILD=PLATFORM ./gradlew build --scan | ||
shell: bash | ||
|
||
- name: Publish to Maven Local | ||
run: VERSION=${{ inputs.branch_version_tag }} SUB_BUILD=PLATFORM ./gradlew publishToMavenLocal | ||
shell: bash | ||
|
||
- name: Login to Docker (on Master) | ||
uses: docker/login-action@v1 | ||
with: | ||
username: airbytebot | ||
password: ${{ inputs.dockerhub_token }} | ||
|
||
- name: Push Docker Images | ||
run: | | ||
# push the minimum set of images required for Cloud | ||
docker push airbyte/worker:${{ inputs.branch_version_tag }} | ||
docker push airbyte/webapp:${{ inputs.branch_version_tag }} | ||
docker push airbyte/metrics-reporter:${{ inputs.branch_version_tag }} | ||
docker push airbyte/scheduler:${{ inputs.branch_version_tag }} | ||
shell: bash |
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.
it would be nice if we made sure that we use the same commands here as we do in the "real" build that actually publishes releases. I worry that if they diverge over time then this won't be an accurate reflection of reality.
@davinchia may have some thoughts on how to share it. Okay if we don't block on this, but let's make sure we consolidate it shortly after so that we don't stay open to the drift.
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 agree it's useful to keep the same docker pushing interface. We could reuse the exact same command as https://github.com/airbytehq/airbyte/blob/master/tools/bin/release_version.sh#L43.
I do think pushing unneeded images is wasteful so I'm leaning towards having a docker-compose-cloud-build.yaml file with a slimmed down image set and using the docker-compose command here.
8d5c1a2
to
82efa83
Compare
// These values are the same for building an specific Airbyte release or branch via the 'VERSION' environment variable. | ||
// For local development builds, the 'VERSION' environment variable is unset, and built images are tagged with 'dev'. | ||
ext { | ||
version = System.getenv("VERSION") ?: env.VERSION |
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.
nice!
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.
can we document this somewhere? maybe this https://docs.airbyte.com/contributing-to-airbyte/developing-locally?
82efa83
to
bc9d5a2
Compare
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.
Nice work!
Couple of non-blocking suggestions from me.
c088026
to
7cfa3d9
Compare
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 looks fantastic, especially given that these are new technologies you dove into. I think Charles and Davin covered the all the nits I saw already. Looks great.
and @cgardens and @davinchia Where if you gif approval game friends?
…ages that are pushed by oss branch action
0d504c4
to
d0d2d43
Compare
@pmossman ping me (on slack please) if you need another look on any of this work |
What
Part of #9651
The changes in this PR aim to make it easy for a developer to prepare an OSS project branch for deployment to a Cloud environment.
How
The key changes in this PR are summarized as follows (and each have a corresponding commit):
VERSION is now a buildArg in our Dockerfiles that reference an AirbyteVersion. This buildArg defaults to the current AirbyteVersion, and is updated via our usual semver process. Now that it's a buildArg, we can overwrite the default to reference build artifacts that aren't tied to a specific Airbyte version. This is important for building and referencing branch-specific artifacts
There is now a single source of truth for the project artifact version and the project image tag. These values are available in the
project.ext
block in the top-level build.gradle, asrootProject.ext.version
androotProject.ext.image_tag
.ext.version
tries to use theVERSION
environment variable, and falls back on.env.VERSION
if unset.ext.image_tag
tries to use theVERSION
environment variable, and falls back ondev
if unset.dev
images, and building official Airbyte Release versions for prod)There is a new Github Action that takes in a branch-specific tag as input, builds the project with that tag as the
VERSION
env var, publishes artifacts to maven local (for the Cloud workflow to consume), and pushes branch-specific docker images to dockerhub (for Cloud to deploy)The AirbyteVersion class has a version validation that allows official semver, or
dev
. This validation has been loosened to also allow versions that containoss-branch
. Without this change, the bootloader won't allow branch-specific versions.Recommended reading order
Misc
Before taking on this task, I had virtually zero Gradle or Github Action experience, so please please point out things that I'm doing inefficiently. A lot of this was trial and error and learning on the fly, so I really appreciate all feedback to help cement my learning in this new territory!
The Cloud PR will soon follow, though I think it makes sense to review this PR first!