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

feat(scripts): push generated docs to material assets repo #2720

Merged

Conversation

riavalon
Copy link
Contributor

Setup scripts for pushing generated docs to material2-docs-content repo on release

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Jan 19, 2017
.travis.yml Outdated
@@ -33,6 +33,7 @@ env:
- MODE=aot
- MODE=payload
- MODE=e2e
- MODE=docs
Copy link
Member

Choose a reason for hiding this comment

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

The whole mode shouldn't be necessary when we run it with the travis-after-modes script.

# If the docs directory is not present, abort
if [ ! -d $docsPath ]; then
echo "Material docs not generated. Aborting..."
exit 0
Copy link
Member

Choose a reason for hiding this comment

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

Why not returning an exit code 1 here? If Travis turns red we can see that there are issues with the docs.

# git remote show origin
git config user.name "$commitAuthorName"
git config user.email "$commitAuthorEmail"
git config credential.helper "store --file=.git/credentials"
Copy link
Member

Choose a reason for hiding this comment

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

Since the Github credentials are now retrieved from the .git/credentials file, the credentials file needs to be created.

echo "https://${MATERIAL2_BUILDS_TOKEN}:@github.com" > .git/credentials

echo "returning to root and removing temp repo"
cd $root
rm -rf $tempRepoPath
echo "Done!"
Copy link
Member

Choose a reason for hiding this comment

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

I don't think that returning to the project directory is necessary here.

  • Those scripts run most of the time in a child process and don't affect the users PWD.

git clone $repoUrl $tempRepoPath

# Clean out repo directory and copy contents of dist/docs into it
$(find $tempRepoPath -d 1 | grep -v "git" | xargs rm -rf)
Copy link
Member

Choose a reason for hiding this comment

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

How about just calling rm -rf $tempRepoPath /*?

The .git folder won't be affected because the wildcard doesn't cover .* files / folders.

@riavalon riavalon force-pushed the feat-add-docs-publishing-script branch 3 times, most recently from f101822 to 9d8847e Compare January 19, 2017 19:03
Copy link
Member

@devversion devversion left a comment

Choose a reason for hiding this comment

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

Code-wise LGTM.

Still thinking about a better name than tmpRepoPath.

cd "$(dirname $0)/../../"

docsPath="./dist/docs"
tempRepoPath="./dist/tempRepo"
Copy link
Member

Choose a reason for hiding this comment

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

Rather than writing to dist/, /tmp/material2-docs-content would be more appropriate (since it's not the output of a build).

I think that just repoDir or repoPath is good.


# Clean out repo directory and copy contents of dist/docs into it
rm -rf $tempRepoPath/*
mkdir $tempRepoPath/docs
Copy link
Member

@jelbourn jelbourn Jan 19, 2017

Choose a reason for hiding this comment

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

I don't think that the content needs to all go into a docs directory. The structure should look something like

material2-docs-content
├─ overview
├─ guides
├─ api
└─ examples

Copy link
Member

Choose a reason for hiding this comment

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

Realized I should add a bit more here; we should:

  • Change the current gulp docs task to be called gulp markdown-docs
  • Change the current gulp api to gulp api-docs
  • Add a new task called gulp docs that just runs the previous two
  • Change gulp api-docs to output to dist/docs/api and markdown-docs to output to dist/docs/markdown

This should make it easier for this script to massage the different types of docs into the right place (and still only need to run one command).

Copy link
Member

Choose a reason for hiding this comment

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

Also while we are talking about naming. Can we rename the file to publish-docs-content.sh?

Artifacts sounds like we are building a release of the components


docsPath="./dist/docs"
tempRepoPath="./dist/tempRepo"
repoUrl="https://github.com/angular/material2-docs-content"
Copy link
Member

Choose a reason for hiding this comment

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

It would be good to let this do a few test runs before committing to the real repo. We did this for the material2-builds; you can do the same by:

  • Fork angular/material2-docs-content
  • Add an access token for said fork to our Travis CI environment
  • Update this script to use that token instead of MATERIAL2_BUILDS_TOKEN

We can then just let it run for a few commits and make sure it looks good before updating the repo / access token.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think everybody can create Travis encrypted variables. I can create a small repo for it and we don't need to change the token at all. (Since its currently running over my token)

Copy link
Member

Choose a reason for hiding this comment

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

@devversion That's probably easier

Copy link
Member

Choose a reason for hiding this comment

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

Okay so here is the repository: https://github.com/DevVersion/material2-docs-content.git

@jelbourn In the future we can probably switch to another token.

@riavalon riavalon force-pushed the feat-add-docs-publishing-script branch from 9d8847e to 23f4f38 Compare January 23, 2017 20:19
Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

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

Just a couple last comments!

cd "$(dirname $0)/../../"

docsPath="./dist/docs"
repoPath="./tmp/material2-docs-content"
Copy link
Member

Choose a reason for hiding this comment

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

Can you change to to /tmp instead of ./tmp?
(here and elsewhere)

overviewFiles=$docsPath/markdown/

# Move api files over to $repoPath/api
cp -r $docsPath/api/ $repoPath/api
Copy link
Member

Choose a reason for hiding this comment

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

It looks like this is creating a structure of material2-docs-assets/api/api (api is in there twice)

@riavalon riavalon force-pushed the feat-add-docs-publishing-script branch from 51bbb77 to ab8dd2d Compare January 25, 2017 15:34
Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

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

LGTM

@jelbourn jelbourn added pr: lgtm action: merge The PR is ready for merge by the caretaker and removed pr: needs review labels Jan 30, 2017
@kara kara added pr: needs rebase and removed action: merge The PR is ready for merge by the caretaker labels Feb 3, 2017
@kara
Copy link
Contributor

kara commented Feb 3, 2017

@riavalon Can you rebase?

@riavalon riavalon force-pushed the feat-add-docs-publishing-script branch from ab8dd2d to ed277d0 Compare February 4, 2017 21:09
@kara kara added action: merge The PR is ready for merge by the caretaker and removed pr: needs rebase labels Feb 6, 2017
@tinayuangao tinayuangao merged commit ba12f44 into angular:master Feb 6, 2017
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants