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

chore(scripts): allow --reset --up/--down to automatically run script #8826

Merged
merged 10 commits into from
Jul 30, 2020

Conversation

BryanPan342
Copy link
Contributor

@BryanPan342 BryanPan342 commented Jul 1, 2020

[ISSUE]
Honestly, kind of miss being able to use lerna for any kind of npm test through parameters with the old builddown. Wanted to be able to run something like builddown build+test or builddown test.

[APPROACH]
Added a -r | --reset flag to allow the foreach.sh script to reset and run in one line like builddown/buildup.

[NOTE]
Won't change anyone's current workflow because --reset is still in codebase.

Also arguments for the script aren't restricted by order (i.e. foreach.sh --reset --up yarn build and builddown yarn build --up --reset will produce the same results)!


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

@BryanPan342
Copy link
Contributor Author

@nija-at @njlynch I saw both #8782 and #8784 and the comments on how buildup and builddown is just to be used as an alias for building. However, I quite liked the previous implementation of builddown with lerna because it allowed for an easy way to run yarn test against the dependencies.

I think our customers contributing to our codebase might find a benefit in simple to use commands that reduce their workflow! Open to suggestions though and if this is something that we want to be pushed to production, I can briefly append to the README.

@BryanPan342 BryanPan342 requested review from njlynch and nija-at and removed request for njlynch July 1, 2020 04:10
@BryanPan342 BryanPan342 added the contribution/core This is a PR that came from AWS. label Jul 1, 2020
Copy link
Contributor

@nija-at nija-at left a comment

Choose a reason for hiding this comment

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

Hey @BryanPan342 -

Thanks for submitting this PR 😊

It seems that the functionality you're looking for already exists. You could run the same thing using foreach.sh --down yarn <command>. It feels better to keep buildup and builddown for the most simple case and use foreach.sh for anything more advanced.

A better contribution/improvement would be in the foreach script instead. foreach needs to be explicitly reset each time by the user. It could be helpful to reset the state when it completes processing the tree successfully. Additionally, a -f or a --force option could be added to reset the state if the user intends to discard previous state regardless of how far it has executed through the tree.
This way, foreach can be executed in a single step, similar to buildup and builddown.

@nija-at nija-at assigned njlynch, nija-at and BryanPan342 and unassigned njlynch and nija-at Jul 1, 2020
@BryanPan342
Copy link
Contributor Author

BryanPan342 commented Jul 1, 2020

A better contribution/improvement would be in the foreach script instead. foreach needs to be explicitly reset each time by the user. It could be helpful to reset the state when it completes processing the tree successfully. Additionally, a -f or a --force option could be added to reset the state if the user intends to discard previous state regardless of how far it has executed through the tree.
This way, foreach can be executed in a single step, similar to buildup and builddown.

Okay yeah this makes a lot of sense.

I think the natural format to tackle this for me:

  • add a -f and --force option that resets state and moves forward
  • keeping the existing --reset in case people just want to reset state for some reason
  • adjusting buildup/builddown so that --resume just adds a tag to the script to flag with -f

Marked this as done because didn't think it would be necessary to change the builddown/buildup scripts

@BryanPan342 BryanPan342 requested a review from nija-at July 1, 2020 19:28
nija-at
nija-at previously requested changes Jul 9, 2020
Copy link
Contributor

@nija-at nija-at left a comment

Choose a reason for hiding this comment

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

Don't forget to update the PR title and description to the new changes 😊

scripts/foreach.sh Outdated Show resolved Hide resolved
scripts/foreach.sh Outdated Show resolved Hide resolved
scripts/foreach.sh Outdated Show resolved Hide resolved
scripts/foreach.sh Outdated Show resolved Hide resolved
@BryanPan342 BryanPan342 changed the title chore(scripts): allow buildup/builddown to run specific yarn argument chore(scripts): add flag in foreach.sh to allow for reset and run Jul 9, 2020
@BryanPan342 BryanPan342 changed the title chore(scripts): add flag in foreach.sh to allow for reset and run chore(scripts): allow --reset --up/--down to automatically run script Jul 13, 2020
@BryanPan342 BryanPan342 requested a review from nija-at July 13, 2020 22:32
@BryanPan342 BryanPan342 requested a review from njlynch July 30, 2020 15:25
@BryanPan342 BryanPan342 dismissed nija-at’s stale review July 30, 2020 16:08

addressed commit body change

@mergify
Copy link
Contributor

mergify bot commented Jul 30, 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).

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: ae5ce32
  • 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 Jul 30, 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 3cf6a94 into aws:master Jul 30, 2020
curtiseppel pushed a commit to curtiseppel/aws-cdk that referenced this pull request Aug 11, 2020
…aws#8826)

**[ISSUE]**
Honestly, kind of miss being able to use `lerna` for any kind of npm test through parameters with the old `builddown`. Wanted to be able to run something like `builddown build+test` or `builddown test`.

**[APPROACH]**
Added a `-r | --reset` flag to allow the `foreach.sh` script to reset and run in one line like `builddown`/`buildup`.

**[NOTE]**
Won't change anyone's current workflow because `--reset` is still in codebase.

Also arguments for the script aren't restricted by order (i.e. `foreach.sh --reset --up yarn build ` and `builddown yarn build --up --reset` will produce the same results)!

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@BryanPan342 BryanPan342 deleted the cdk-scripts branch September 8, 2020 18:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution/core This is a PR that came from AWS.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants