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

fix(cli): Linux browser not supported for cdk docs #9549

Merged
merged 2 commits into from
Sep 1, 2020
Merged

fix(cli): Linux browser not supported for cdk docs #9549

merged 2 commits into from
Sep 1, 2020

Conversation

gmeligio
Copy link
Contributor

@gmeligio gmeligio commented Aug 9, 2020

fix(cli): Add browser command for Linux

Adding "xdg-open %u" that is available in all free-desktop distros, so it can be used as the standard docs command for Linux users

Fixes #2847


  • By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

Copy link
Contributor

@shivlaks shivlaks left a comment

Choose a reason for hiding this comment

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

thanks for opening a PR for this use case! changes seem fine to me, but there are a couple things we should fix up before merging this one that are not related to the code.

  1. let's move the integration test output as a comment in this PR so it's not a part of the commit body.
    Rationale: the commit body should describe the problem we're solving, the approach, considerations, and any other pertinent implementation details

  2. can you move the Fixes #2847 into the commit body

These should be covered in our CONTRIBUTING guide

@gmeligio
Copy link
Contributor Author

Ok. Great!

This is the final output of the integration tests:

yarn integ-cli-no-regression
 FAIL  ./cli.integtest.js (1792.941s)
  ✓ VPC Lookup (368932ms)
  ✓ Two ways of shoing the version (2393ms)
  ✓ Termination protection (48197ms)
  ✓ cdk synth (4577ms)
  ✓ ssm parameter provider error (3921ms)
  ✓ automatic ordering (76561ms)
  ✓ context setting (3866ms)
  ✓ deploy (41119ms)
  ✓ deploy all (69434ms)
  ✓ nested stack with parameters (62919ms)
  ✓ deploy without execute (16659ms)
  ✓ security related changes without a CLI are expected to fail (3461ms)
  ✓ deploy wildcard with outputs (69485ms)
  ✓ deploy with parameters (39009ms)
  ✓ update to stack in ROLLBACK_COMPLETE state will delete stack and create a new one (104364ms)
  ✓ stack in UPDATE_ROLLBACK_COMPLETE state can be updated (124623ms)
  ✓ deploy with wildcard and parameters (100257ms)
  ✓ deploy with parameters multi (39256ms)
  ✓ deploy with notification ARN (40052ms)
  ✓ deploy with role (59424ms)
  ✓ cdk diff (7561ms)
  ✓ cdk diff --fail on multiple stacks exits with error if any of the stacks contains a diff (46609ms)
  ✓ cdk diff --fail with multiple stack exits with if any of the stacks contains a diff (46432ms)
  ✕ deploy stack with docker asset (6924ms)
  ✓ deploy and test stack with lambda asset (51976ms)
  ✓ cdk ls (2866ms)
  ✓ deploy stack without resource (65438ms)
  ✓ IAM diff (3053ms)
  ✓ fast deploy (135993ms)
  ✓ failed deploy does not hang (38627ms)
  ✓ can still load old assemblies (6138ms)
  ✕ generating and loading assembly (92173ms)

  ● deploy stack with docker asset

    'cdk deploy --require-approval=never cdktest-20200807224618049-docker' exited with error code 1: cdktest-20200807224618049-docker: deploying...
    [0%] start: Publishing cee6dd26fd3f022a0cd4af2785664b82d39730a3cfed8ab78db50f3a1c32da09:current
    Cannot connect to the Docker daemon at unix:///var/run/docker.sock. Is the docker daemon running?
    [100%] fail: docker build --tag cdkasset-cee6dd26fd3f022a0cd4af2785664b82d39730a3cfed8ab78db50f3a1c32da09 . exited with error code 1: Cannot connect to the Docker daemon at unix:///var/run/docker.sock. Is the docker daemon running?

     ❌  cdktest-20200807224618049-docker failed: Error: Failed to publish one or more assets. See the error messages above for more information.
    Failed to publish one or more assets. See the error messages above for more information.

      213 |         resolve((Buffer.concat(stdout).toString('utf-8') + Buffer.concat(stderr).toString('utf-8')).trim());
      214 |       } else {
    > 215 |         reject(new Error(`'${command.join(' ')}' exited with error code ${code}: ${Buffer.concat(stderr).toString('utf-8').trim()}`));
          |                ^
      216 |       }
      217 |     });
      218 |   });

      at ChildProcess.child.once.code (cdk-helpers.ts:215:16)

  ● generating and loading assembly

    'cdk deploy --require-approval=never -a . cdktest-20200807224618049-docker-with-custom-file' exited with error code 1: cdktest-20200807224618049-docker-with-custom-file: deploying...
    [0%] start: Publishing fbf8fae1940ad4eea14befbb3e09f983ef55653dbe8e7e7af2b3734246f1b2aa:current
    Cannot connect to the Docker daemon at unix:///var/run/docker.sock. Is the docker daemon running?
    [100%] fail: docker build --tag cdkasset-fbf8fae1940ad4eea14befbb3e09f983ef55653dbe8e7e7af2b3734246f1b2aa --file Dockerfile.Custom . exited with error code 1: Cannot connect to the Docker daemon at unix:///var/run/docker.sock. Is the docker daemon running?

     ❌  cdktest-20200807224618049-docker-with-custom-file failed: Error: Failed to publish one or more assets. See the error messages above for more information.
    Failed to publish one or more assets. See the error messages above for more information.

      213 |         resolve((Buffer.concat(stdout).toString('utf-8') + Buffer.concat(stderr).toString('utf-8')).trim());
      214 |       } else {
    > 215 |         reject(new Error(`'${command.join(' ')}' exited with error code ${code}: ${Buffer.concat(stderr).toString('utf-8').trim()}`));
          |                ^
      216 |       }
      217 |     });
      218 |   });

      at ChildProcess.child.once.code (cdk-helpers.ts:215:16)

Test Suites: 1 failed, 1 of 2 total
Tests:       2 failed, 30 passed, 32 total
Snapshots:   0 total
Time:        1792.988s
Ran all test suites.
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
yarn integ-cli-regression
 FAIL  ./cli.integtest.js (1872.584s)
  ✓ VPC Lookup (438885ms)
  ✓ Two ways of shoing the version (2622ms)
  ✓ Termination protection (49896ms)
  ✓ cdk synth (4849ms)
  ✓ ssm parameter provider error (4543ms)
  ✓ automatic ordering (82916ms)
  ✓ context setting (4085ms)
  ✓ deploy (41058ms)
  ✓ deploy all (69309ms)
  ✓ nested stack with parameters (61910ms)
  ✓ deploy without execute (16672ms)
  ✓ security related changes without a CLI are expected to fail (3451ms)
  ✓ deploy wildcard with outputs (69512ms)
  ✓ deploy with parameters (39308ms)
  ✓ update to stack in ROLLBACK_COMPLETE state will delete stack and create a new one (77173ms)
  ✓ stack in UPDATE_ROLLBACK_COMPLETE state can be updated (124505ms)
  ✓ deploy with wildcard and parameters (100256ms)
  ✓ deploy with parameters multi (39285ms)
  ✓ deploy with notification ARN (40142ms)
  ✓ deploy with role (59509ms)
  ✓ cdk diff (7485ms)
  ✓ cdk diff --fail on multiple stacks exits with error if any of the stacks contains a diff (46585ms)
  ✓ cdk diff --fail with multiple stack exits with if any of the stacks contains a diff (46409ms)
  ✕ deploy stack with docker asset (5911ms)
  ✓ deploy and test stack with lambda asset (51431ms)
  ✓ cdk ls (2413ms)
  ✓ deploy stack without resource (65312ms)
  ✓ IAM diff (3005ms)
  ✓ fast deploy (130221ms)
  ✓ failed deploy does not hang (54730ms)
  ✓ can still load old assemblies (6001ms)
  ✕ generating and loading assembly (102568ms)

  ● deploy stack with docker asset

    'cdk deploy --require-approval=never cdktest-20200807232046901-docker' exited with error code 1: cdktest-20200807232046901-docker: deploying...
    [0%] start: Publishing cee6dd26fd3f022a0cd4af2785664b82d39730a3cfed8ab78db50f3a1c32da09:current
    Cannot connect to the Docker daemon at unix:///var/run/docker.sock. Is the docker daemon running?
    [100%] fail: docker build --tag cdkasset-cee6dd26fd3f022a0cd4af2785664b82d39730a3cfed8ab78db50f3a1c32da09 . exited with error code 1: Cannot connect to the Docker daemon at unix:///var/run/docker.sock. Is the docker daemon running?

     ❌  cdktest-20200807232046901-docker failed: Error: Failed to publish one or more assets. See the error messages above for more information.
    Failed to publish one or more assets. See the error messages above for more information.

      at ChildProcess.child.once.code (cdk-helpers.ts:215:16)

  ● generating and loading assembly

    'cdk deploy --require-approval=never -a . cdktest-20200807232046901-docker-with-custom-file' exited with error code 1: cdktest-20200807232046901-docker-with-custom-file: deploying...
    [0%] start: Publishing fbf8fae1940ad4eea14befbb3e09f983ef55653dbe8e7e7af2b3734246f1b2aa:current
    Cannot connect to the Docker daemon at unix:///var/run/docker.sock. Is the docker daemon running?
    [100%] fail: docker build --tag cdkasset-fbf8fae1940ad4eea14befbb3e09f983ef55653dbe8e7e7af2b3734246f1b2aa --file Dockerfile.Custom . exited with error code 1: Cannot connect to the Docker daemon at unix:///var/run/docker.sock. Is the docker daemon running?

     ❌  cdktest-20200807232046901-docker-with-custom-file failed: Error: Failed to publish one or more assets. See the error messages above for more information.
    Failed to publish one or more assets. See the error messages above for more information.

      at ChildProcess.child.once.code (cdk-helpers.ts:215:16)

Test Suites: 1 failed, 1 of 2 total
Tests:       2 failed, 30 passed, 32 total
Snapshots:   0 total
Time:        1872.633s
Ran all test suites.
+ cleanup
+ cp /workspace/aws-cdk/packages/aws-cdk/test/integ/cli-backwards-tests-v1.57.0/coverage/junit.xml .
+ rm -rf /tmp/tmp.V8HPu4BGLi
+ rm -rf /workspace/aws-cdk/packages/aws-cdk/test/integ/cli-backwards-tests-v1.57.0
npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! aws-cdk@0.0.0 integ-cli-regression-latest-release: `test/integ/run-against-repo test/integ/test-cli-regression-against-latest-release.sh`
npm ERR! Exit status 1
npm ERR! 
npm ERR! Failed at the aws-cdk@0.0.0 integ-cli-regression-latest-release script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

npm ERR! A complete log of this run can be found in:
npm ERR!     /home/gitpod/.npm/_logs/2020-08-07T23_51_58_826Z-debug.log
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

Adding "xdg-open %u" that is available in all free-desktop distros, so it can be used as the standard docs command for Linux users

Fixes #2847
@gmeligio gmeligio changed the title fix(cli): Add browser command for Linux. Fixes #2847 fix(cli): Add browser command for Linux Aug 21, 2020
@gmeligio
Copy link
Contributor Author

  1. can you move the Fixes #2847 into the commit body

These should be covered in our CONTRIBUTING guide

It is indeed covered in the our CONTRIBUTING guide. I misplaced it. I think that pulling the last changes from upstream master and updating the commit message with a forced push I corrupted the pull request. Shoud I do anything else?

@shivlaks
Copy link
Contributor

@gmeligio I think something is off with the latest merge - don't think you intended to change 877 files. can you update and merge again (avoid force pushing as it can be problematic).

@gmeligio gmeligio closed this Aug 26, 2020
@gmeligio gmeligio reopened this Aug 26, 2020
@mergify mergify bot dismissed shivlaks’s stale review August 26, 2020 11:50

Pull request has been modified.

@gmeligio
Copy link
Contributor Author

gmeligio commented Aug 26, 2020

  1. can you move the Fixes #2847 into the commit body

I think that the history is fixed and we are now where we wanted to be before the mistaken push. Does it seem fine to you?

@shivlaks shivlaks added the pr-linter/exempt-test The PR linter will not require test changes label Aug 28, 2020
@shivlaks shivlaks changed the title fix(cli): Add browser command for Linux fix(cli): add browser command for Linux users of cdk docs Aug 28, 2020
@shivlaks shivlaks changed the title fix(cli): add browser command for Linux users of cdk docs fix(cli): add Linux browser support for cdk docs Aug 28, 2020
shivlaks
shivlaks previously approved these changes Aug 28, 2020
@shivlaks shivlaks changed the title fix(cli): add Linux browser support for cdk docs fix(cli): Linux browser not supported for cdk docs Aug 28, 2020
@mergify mergify bot dismissed shivlaks’s stale review September 1, 2020 06:09

Pull request has been modified.

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: 0a13d6e
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@mergify
Copy link
Contributor

mergify bot commented Sep 1, 2020

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@mergify mergify bot merged commit 663913f into aws:master Sep 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-linter/exempt-test The PR linter will not require test changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"cdk docs" platform-specific browser command
3 participants