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

docs(contributing): update Gitpod guidance #10380

Closed
wants to merge 2 commits into from
Closed

docs(contributing): update Gitpod guidance #10380

wants to merge 2 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Sep 15, 2020

Update Gitpod guidance in contribution guidelines.


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

@ghost ghost requested a review from skinny85 September 15, 2020 23:49
@mergify
Copy link
Contributor

mergify bot commented Sep 15, 2020

Title does not follow the guidelines of Conventional Commits. Please adjust title before merge.

@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Sep 15, 2020
@ghost ghost changed the title Update Gitpod guidance Docs: Update Gitpod guidance Sep 15, 2020
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

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

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

@ghost ghost changed the title Docs: Update Gitpod guidance docs(contributing): update Gitpod guidance Sep 17, 2020
Copy link
Contributor

@skinny85 skinny85 left a comment

Choose a reason for hiding this comment

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

A few thoughts/suggestions.

we recommend using [Gitpod](http://gitpod.io) -
a service that allows you to spin up an in-browser
Visual Studio Code-compatible editor,
with everything set up and ready to go for CDK development.
Best of all, the project is already built.
Copy link
Contributor

Choose a reason for hiding this comment

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

I would move this below (where it discusses the build currently).

Comment on lines +64 to +65
This will start a new Gitpod workspace.
You can now work on any package that you want to modify,
Copy link
Contributor

Choose a reason for hiding this comment

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

While I agree the current wording is incorrect, we need more subtlety here, as it's possible in some cases that the workspace will not be pre-built, and the user will have to wait for the build to complete. See my blog article on this for details: https://www.endoflineblog.com/cdk-tips-02-how-to-contribute-to-the-cdk#gitpod-environment .

Maybe we can use a similar wording to that article (feel free to improve it of course 🙂):

(In rare cases, you might create a workspace just after a commit to the main branch has been pushed; since a full build of the project takes around 45 minutes, there is a short window of time after a push where the pre-build is not ready. In that case, you might have to wait a bit for the build to finish.)

Comment on lines +70 to +75
We don't recommend doing a full build on GitPod; it takes over
an hour. Also, it will fail unless you include `--skip-prereqs`
on the `yarn build` command (because Docker doesn't work in a
Gitpod workspace). Instead, just discard your Gitpod workspace
and create a new one. You will get the latest source files and
bulid artifacts.
Copy link
Contributor

Choose a reason for hiding this comment

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

How do you feel about moving this paragraph to the ### Main build scripts section below? Not sure it fits here.

Comment on lines +67 to +68
You can build and test only the module you're working on and its
consumers using `scripts/builddown`.
Copy link
Contributor

Choose a reason for hiding this comment

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

How about instead of this, we link to the #partial-build section below?

@skinny85
Copy link
Contributor

This was subsumed by #13525.

@skinny85 skinny85 closed this Mar 16, 2021
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.

2 participants