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

Deduplicate environment variables in CI #14351

Merged
merged 3 commits into from
Jun 27, 2023

Conversation

cameel
Copy link
Member

@cameel cameel commented Jun 23, 2023

While working on #13583 I initially made some simple mistakes when defining environment variables for CI jobs, and realized that we can do it better to make such mistakes less likely.

This PR is a simple refactor to make jobs extend the set of environment variables defined in base image definitions instead of redefining them.

Details

Currently, when we want an extra variable for a job, but still need the variables we defined for the base image, we repeat them like this:

defaults:
  - base_ubuntu_clang: &base_ubuntu_clang
      docker:
        - image: << pipeline.parameters.ubuntu-clang-docker-image >>
      environment:
        TERM: xterm
        CC: clang
        CXX: clang++
        MAKEFLAGS: -j 3

jobs:
  b_ubu_clang:
    <<: *base_ubuntu_clang
    environment:
      TERM: xterm
      CC: clang
      CXX: clang++
      MAKEFLAGS: -j 3
      CMAKE_OPTIONS: << parameters.cmake_options >>

This is error-prone and verbose. In many cases we don't even repeat variables a job does not use, to make the job definition shorter.

The PR replaces this convention with reuse of environment dicts via YAML anchors like this:

defaults:
  - base_ubuntu_clang: &base_ubuntu_clang
      docker:
        - image: << pipeline.parameters.ubuntu-clang-docker-image >>
      environment: &base_ubuntu_clang_env
        TERM: xterm
        CC: clang
        CXX: clang++
        MAKEFLAGS: -j 3

jobs:
  b_ubu_clang:
    <<: *base_ubuntu_clang
    environment:
      <<: *base_ubuntu_clang_env
      CMAKE_OPTIONS: << parameters.cmake_options >>

@cameel cameel requested a review from r0qs June 23, 2023 11:40
@cameel cameel self-assigned this Jun 23, 2023
@cameel cameel force-pushed the ci-deduplicate-base-env-with-anchors branch from cb48017 to d303ff9 Compare June 23, 2023 16:37
Copy link
Member

@r0qs r0qs left a comment

Choose a reason for hiding this comment

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

LGTM just need a rebase :)

- Also remove variables that are now redundant.
- Note that this adds new variables for jobs that previously did not have them. This should not affect behavior because we were simply omitting those variables where they were not necessary.
@cameel cameel force-pushed the ci-deduplicate-base-env-with-anchors branch from d303ff9 to 8b64195 Compare June 27, 2023 12:00
@cameel
Copy link
Member Author

cameel commented Jun 27, 2023

Rebased.

@cameel cameel requested a review from r0qs June 27, 2023 12:02
@cameel cameel enabled auto-merge June 27, 2023 12:32
@cameel cameel merged commit 3bb492a into develop Jun 27, 2023
@cameel cameel deleted the ci-deduplicate-base-env-with-anchors branch June 27, 2023 12:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants