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

Propagate environment #1358 #1392

Merged
merged 27 commits into from
Jan 23, 2024
Merged

Conversation

ajain58
Copy link
Contributor

@ajain58 ajain58 commented Jan 17, 2024

Adds propagate-environment: true to all pipeline templates and removes the environment definition from docker compose files

Closes #1358

@ajain58 ajain58 requested review from a team as code owners January 17, 2024 00:45
Copy link

changeset-bot bot commented Jan 17, 2024

🦋 Changeset detected

Latest commit: 10ddc21

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
skuba Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@samchungy samchungy self-assigned this Jan 17, 2024
Copy link
Member

@72636c 72636c left a comment

Choose a reason for hiding this comment

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

Great find. Looks like a straightforward port of the Docker plugin implementation buildkite-plugins/docker-compose-buildkite-plugin#344 so I'm not seeing any additional risks.

template/express-rest-api/docker-compose.yml Show resolved Hide resolved
template/lambda-sqs-worker/docker-compose.yml Show resolved Hide resolved
docs/deep-dives/github.md Outdated Show resolved Hide resolved
Copy link
Member

@72636c 72636c left a comment

Choose a reason for hiding this comment

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

👏

We should add a changeset for this

docs/deep-dives/github.md Outdated Show resolved Hide resolved
docs/deep-dives/github.md Outdated Show resolved Hide resolved
@ajain58
Copy link
Contributor Author

ajain58 commented Jan 19, 2024

👏

We should add a changeset for this

this would be a minor right @72636c ?

@72636c
Copy link
Member

72636c commented Jan 19, 2024

@ajain58 Great question, we have an informal & impure 🙃 practice of filing template updates as patches. This is dishonest to semver but the rationale is that existing projects are the ones that upgrade to new versions & review release notes, and once you've run skuba init then updates to the built-in templates don't really impact you beyond an FYI about about a best current practice.

@72636c
Copy link
Member

72636c commented Jan 19, 2024

72636c added a commit that referenced this pull request Jan 19, 2024
72636c added a commit that referenced this pull request Jan 20, 2024
environment:
- ENVIRONMENT
- GITHUB_API_TOKEN
- VERSION
Copy link
Contributor

Choose a reason for hiding this comment

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

Any ideas where VERSION comes from? @72636c. I can see ENVIRONMENT in the UI so it will get propagated successfully by the propagate-environment setting.

Copy link
Contributor

@samchungy samchungy Jan 23, 2024

Choose a reason for hiding this comment

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

VERSION: ${BUILDKITE_COMMIT:0:7}.${BUILDKITE_BUILD_NUMBER}

Is this template meant to have this line?

Copy link
Member

Choose a reason for hiding this comment

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

Ah yes, I think that's right re: VERSION. Maybe we should extend the CDK template to use the version variable too

CDK

const logger = createLogger({
name: '<%- serviceName %>',
});

Serverless

version: Env.string('VERSION'),

base: {
environment: config.environment,
version: config.version,
},

@samchungy
Copy link
Contributor

I did a quick test and pipeline variables which are defined within steps and/or on the pipeline at upload time also get passed to the container via the propagate-environment option.

Copy link
Member

@72636c 72636c left a comment

Choose a reason for hiding this comment

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

Thanks a bunch for this 🙌

@72636c 72636c merged commit 3a99839 into seek-oss:master Jan 23, 2024
8 checks passed
@seek-oss-ci seek-oss-ci mentioned this pull request Jan 23, 2024
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.

Use propagate-environment: true in Docker Compose config
3 participants