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

582 602 expected docker compose err #794

Merged
merged 8 commits into from Jan 9, 2023

Conversation

devinsag
Copy link
Contributor

@devinsag devinsag commented Jan 4, 2023

Overview

Please see:

https://gitlab.com/architect-io/architect-cli/-/issues/582

https://gitlab.com/architect-io/architect-cli/-/issues/602

Changes

Add a check to see if yaml is empty before parsing.

  • Throws error if empty.

Add try catch around dev buildImage function.

  • Catch identified error message and display message to screen.

Tests

run dev command against empty architect.yml file. See error message, and no sentry submission
run dev command with a dockerfile containing an exception. See error message, and no sentry submission.

test
    .timeout(20000)
    .stub(ComponentBuilder, 'loadFile', () => {
      return '';
    })
    .stub(Dev.prototype, 'failIfEnvironmentExists', sinon.stub().returns(undefined))
    .stub(Dev.prototype, 'runCompose', sinon.stub().returns(undefined))
    .stub(Dev.prototype, 'downloadSSLCerts', sinon.stub().returns(undefined))
    .stub(Dev.prototype, 'readSSLCert', sinon.stub().returns('fake-cert'))
    .stdout({ print })
    .stderr({ print })
    .command(['dev', './examples/hello-world/architect.yml'])
    .catch(err => {
      expect(err.message).to.include('For help getting started take a look at our documentation here: https://docs.architect.io/reference/architect-yml');
    })
    .it('Provide error if architect.yml is empty');

Pictures

Docker compose error

image

Empty architect.yml

image

try {
await DockerComposeUtils.dockerCompose(['-f', compose_file, '-p', project_name, 'build', ...build_args], { stdio: 'inherit' });
} catch (e: any) {
const pattern = new RegExp('Command failed with exit code 17: docker compose -f [-p architect build]?', 'g');
Copy link
Contributor

Choose a reason for hiding this comment

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

This pattern isn't sufficient, if I run architect dev -e foo the failure is

Command failed with exit code 17: docker compose -f /Users/tyler/.config/architect/local/architect/architect/docker-compose/foo.yml -p foo build

the -p argument is equivalent to the environment we pass in.

We should be able to get the exit code from e.exitCode and check that value instead. Is exit code 17 the only possibility / the only exit code we care about? I imagine any non-zero exit code from docker compose build wouldn't be something we can fix on our end, but not 100% sure of that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated logic to fetch docker compose exit code e.exitCode instead of parsing error message. Updated to throw error when non-zero exit code.

Comment on lines 509 to 512
try {
cmd.stderr?.on('data', (bytes) => {
stderr_message = (bytes.toString() || '').trim();
});
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm pretty sure this information is in the exception object returned by execa - I believe e.stderris available so we don't need to set the string this way. You can run console.log(e) to quickly see the various keys available - but I think that would be the same information that's being captured here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed on-data function

@TylerAldrich TylerAldrich merged commit a769260 into rc Jan 9, 2023
@TylerAldrich TylerAldrich deleted the 582-expected-docker-compose-err branch January 9, 2023 16:25
github-actions bot pushed a commit that referenced this pull request Jan 9, 2023
## [1.31.2-rc.1](v1.31.1...v1.31.2-rc.1) (2023-01-09)

### Bug Fixes

* **dev:** Improve error message when docker build fails and prevent sending error reports ([#794](#794)) ([a769260](a769260))
github-actions bot pushed a commit that referenced this pull request Jan 11, 2023
## [1.31.2-arc-skypack2.1](v1.31.1...v1.31.2-arc-skypack2.1) (2023-01-11)

### Bug Fixes

* **dev:** Improve error message when docker build fails and prevent sending error reports ([#794](#794)) ([a769260](a769260))
github-actions bot pushed a commit that referenced this pull request Jan 20, 2023
# [1.32.0](v1.31.1...v1.32.0) (2023-01-20)

### Bug Fixes

* **auth:** Add timeout to getToken ([#799](#799)) ([5e5963b](5e5963b))
* **clusters:** Incorrect cluster command when warning about deprecated platform command ([#796](#796)) ([3e5a74a](3e5a74a))
* **dev:** Improve error message when docker build fails and prevent sending error reports ([#794](#794)) ([a769260](a769260))
* **register:** Add new accept header used by buildx 0.10.0 ([#803](#803)) ([84706f4](84706f4))
* **validate:** Upgrade class-transformer and class-validator ([#797](#797)) ([647f880](647f880))
* **volumes:** Disable volumes for remote deploys ([#800](#800)) ([68cf984](68cf984))

### Features

* **dev:** Add support for optional services ([#801](#801)) ([25d229b](25d229b))
@github-actions
Copy link

🎉 This PR is included in version 1.32.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.

None yet

2 participants