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

Convert compose interpolations #881

Merged
merged 6 commits into from
Apr 18, 2023
Merged

Conversation

mtgoncalves1
Copy link
Contributor

Overview

Closes https://gitlab.com/architect-io/architect-cli/-/issues/467.
Make sure docker compose interpolation gets converted to our syntax correctly.

Changes

  • Convert any compose environment variables with the format of ${*} to Architect format ${{ * }} and create a secret for each of them

Tests

docker-compose.yml

version: '3.2'
services:
  db:
    image: "postgres:${POSTGRES_VERSION}"
    environment:
      POSTGRES_USER: ${DB_USER}
      POSTGRES_PASSWORD: ${DB_PASSWORD}
      POSTGRES_DB: ${DB_NAME}

Run the init command. It should result in the following architect file.
$ architect-local init --from-compose=docker-compose.yml test

name: test
services:
  db:
    image: postgres:${{ secrets.POSTGRES_VERSION }}
    environment:
      POSTGRES_USER: ${{ secrets.DB_USER }}
      POSTGRES_PASSWORD: ${{ secrets.DB_PASSWORD }}
      POSTGRES_DB: ${{ secrets.DB_NAME }}
    reserved_name: db
secrets:
  POSTGRES_VERSION:
    required: true
  DB_USER:
    required: true
  DB_PASSWORD:
    required: true
  DB_NAME:
    required: true

@mtgoncalves1 mtgoncalves1 requested a review from mueschm April 7, 2023 15:55
@mtgoncalves1 mtgoncalves1 self-assigned this Apr 7, 2023
@mueschm
Copy link
Member

mueschm commented Apr 10, 2023

The server_online.sh script is not working. More than likely, the number of services changed due to the test update. Should be easy and a good learning experience to update the script and fix everything.

@tjhiggins
Copy link
Member

The server_online.sh script is not working. More than likely, the number of services changed due to the test update. Should be easy and a good learning experience to update the script and fix everything.

I had the same issue. Which is how I went down the --wait path. This pr should fix the issue:
#880

@ryan-cahill
Copy link
Member

ryan-cahill commented Apr 12, 2023

It looks like we should take three more cases into account here, $VARIABLE, ${VARIABLE:-default}, and ${VARIABLE:?err}. The last two are convenient in that they use either default or required environment variables and match up well with our spec. Also because of the fact that a syntax to require environment variables exists in a docker-compose file, I think that secrets specified as just ${VARIABLE} or $VARIABLE shouldn't be required secrets. From the docs:

Both $VARIABLE and ${VARIABLE} syntax are supported. Additionally when using the 2.1 file format, it is possible to provide inline default values using typical shell syntax:

${VARIABLE:-default} evaluates to default if VARIABLE is unset or empty in the environment.
${VARIABLE-default} evaluates to default only if VARIABLE is unset in the environment.
Similarly, the following syntax allows you to specify mandatory variables:

${VARIABLE:?err} exits with an error message containing err if VARIABLE is unset or empty in the environment.
${VARIABLE?err} exits with an error message containing err if VARIABLE is unset in the environment.

Also please add tests to test/commands/init.test.ts for these cases

@mtgoncalves1
Copy link
Contributor Author

mtgoncalves1 commented Apr 12, 2023

I think that secrets specified as just ${VARIABLE} or $VARIABLE shouldn't be required secrets.
Ok. I'll leave them as

secrets:
  VARIABLE:
    default:

@ryan-cahill
Copy link
Member

I think that secrets specified as just ${VARIABLE} or $VARIABLE shouldn't be required secrets.
Ok. I'll leave them as

secrets:
  VARIABLE:
    default:

I think that rather than an empty default property, required: false might be more appropriate

@mtgoncalves1
Copy link
Contributor Author

I think that secrets specified as just ${VARIABLE} or $VARIABLE shouldn't be required secrets.
Ok. I'll leave them as

secrets:
  VARIABLE:
    default:

I think that rather than an empty default property, required: false might be more appropriate

Sounds good.

src/common/docker-compose/converter.ts Outdated Show resolved Hide resolved
src/common/docker-compose/converter.ts Outdated Show resolved Hide resolved
@mueschm mueschm removed their request for review April 17, 2023 15:43
@ryan-cahill ryan-cahill merged commit 6c30882 into rc Apr 18, 2023
@ryan-cahill ryan-cahill deleted the 467-compose-env-var-conversion branch April 18, 2023 14:15
github-actions bot pushed a commit that referenced this pull request Apr 18, 2023
# [1.39.0-rc.1](v1.38.1-rc.1...v1.39.0-rc.1) (2023-04-18)

### Features

* **init:** Handle Docker Compose environment variable interpolation formats ([#881](#881)) ([6c30882](6c30882))
github-actions bot pushed a commit that referenced this pull request Apr 20, 2023
# [1.39.0-arc-custom-ingress-tls-3.2](v1.39.0-arc-custom-ingress-tls-3.1...v1.39.0-arc-custom-ingress-tls-3.2) (2023-04-20)

### Bug Fixes

* **dev:** Require dev port selected is 80, 443, or between 1024-65535  ([#895](#895)) ([c35a27b](c35a27b))
* **error:** Provide better error messages when you have conflicting subdomains ([3d8b2e1](3d8b2e1))
* **spec:** Add safe float type to avoid precision loss on yaml.load ([#894](#894)) ([e14e0cd](e14e0cd))

### Features

* **dev:** Automatic cache cleanup ([#857](#857)) ([8b46457](8b46457))
* **init:** Handle Docker Compose environment variable interpolation formats ([#881](#881)) ([6c30882](6c30882))
* **tls:** Added tls support ([b42ab75](b42ab75))
github-actions bot pushed a commit that referenced this pull request Apr 26, 2023
# [1.39.0](v1.38.0...v1.39.0) (2023-04-26)

### Bug Fixes

* **dev:** Require dev port selected is 80, 443, or between 1024-65535  ([#895](#895)) ([c35a27b](c35a27b))
* **error:** Provide better error messages when you have conflicting subdomains ([3d8b2e1](3d8b2e1))
* **exec:** Catch error when trying to hit accounts endpoint and fallback to allowing user to select a local env ([#897](#897)) ([8cd0732](8cd0732))
* **revert:** Revert deprecation warnings for liveness_probe keys ([cc7653d](cc7653d))
* **spec:** Add safe float type to avoid precision loss on yaml.load ([#894](#894)) ([e14e0cd](e14e0cd))

### Features

* **dev:** Automatic cache cleanup ([#857](#857)) ([8b46457](8b46457))
* **init:** Handle Docker Compose environment variable interpolation formats ([#881](#881)) ([6c30882](6c30882))
* **spec:** deprecate liveness_probe port and path keys ([#896](#896)) ([9e2104c](9e2104c))
@github-actions
Copy link

🎉 This PR is included in version 1.39.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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.

4 participants