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

ref(docker): remove all unrequired docker arguments from CI/CD pipelines #7231

Merged
merged 6 commits into from
Jul 25, 2023

Conversation

gustavovalverde
Copy link
Member

@gustavovalverde gustavovalverde commented Jul 17, 2023

Motivation

This is a follow-up work based on:

Considering we refactor all the ARGs and ENVs from the Dockerfile, we had a pending task of syncing those changes with the pipelines.

Complex Code or Requirements

Some inputs were being used for build arguments which are no longer required, in most scenarios those were using the same defaults we have on our Dockerfile, and thus were removed to reduce confusion for other engineers which might see those as a requirement.

The NETWORK has been removed from most inputs, and instead moved to the place where it should be: Docker environment variables (ENV) at runtime. Based on our entrypoint approach, this is safer.

Solution

  • Remove build arguments which are not required from the calling action and the image build workflow
  • Remove environment variables if those are build arguments, like SHORT_SHA (which is still part of the VM name)
  • Prefer using -e (Docker ENV) instead of inputs or build arguments for $NETWORK

Review

Anyone from DevOps, but it might be easier for @upbqdn or Teor, as they participated in the previous review.

Reviewer Checklist

  • Will the PR name make sense to users?
    • Does it need extra CHANGELOG info? (new features, breaking changes, large changes)
  • Are the PR labels correct?
  • Does the code do what the ticket and PR says?
    • Does it change concurrent code, unsafe code, or consensus rules?
  • How do you know it works? Does it have tests?

Follow Up Work

None

@gustavovalverde gustavovalverde added A-infrastructure Area: Infrastructure changes A-devops Area: Pipelines, CI/CD and Dockerfiles C-enhancement Category: This is an improvement P-Medium ⚡ I-usability Zebra is hard to understand or use labels Jul 17, 2023
@gustavovalverde gustavovalverde self-assigned this Jul 17, 2023
@github-actions github-actions bot added C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG extra-reviews This PR needs at least 2 reviews to merge labels Jul 17, 2023
@arya2 arya2 added do-not-merge Tells Mergify not to merge this PR and removed do-not-merge Tells Mergify not to merge this PR labels Jul 21, 2023
@upbqdn upbqdn marked this pull request as ready for review July 22, 2023 22:34
@upbqdn upbqdn requested a review from a team as a code owner July 22, 2023 22:34
@upbqdn upbqdn requested review from arya2 and removed request for a team and arya2 July 22, 2023 22:34
@upbqdn upbqdn marked this pull request as draft July 22, 2023 22:45
upbqdn
upbqdn previously approved these changes Jul 24, 2023
Copy link
Member

@upbqdn upbqdn left a comment

Choose a reason for hiding this comment

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

utACK

.github/workflows/continous-integration-docker.yml Outdated Show resolved Hide resolved
@gustavovalverde gustavovalverde marked this pull request as ready for review July 24, 2023 11:46
@gustavovalverde
Copy link
Member Author

This will need re-approval @upbqdn

@gustavovalverde gustavovalverde requested a review from upbqdn July 24, 2023 11:46
upbqdn
upbqdn previously approved these changes Jul 24, 2023
@gustavovalverde
Copy link
Member Author

@upbqdn sorry to ping you again for re-approval. I had to fix a typo as a tests was failing.

@upbqdn upbqdn self-requested a review July 25, 2023 19:20
@gustavovalverde gustavovalverde removed the extra-reviews This PR needs at least 2 reviews to merge label Jul 25, 2023
mergify bot added a commit that referenced this pull request Jul 25, 2023
@mergify mergify bot merged commit da2e696 into main Jul 25, 2023
@mergify mergify bot deleted the sync-pipelines-with-docker branch July 25, 2023 22:50
@oxarbitrage oxarbitrage mentioned this pull request Sep 1, 2023
40 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-devops Area: Pipelines, CI/CD and Dockerfiles A-infrastructure Area: Infrastructure changes C-enhancement Category: This is an improvement C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG I-usability Zebra is hard to understand or use
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants