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.

Sign up for GitHub

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

Jump to bottom

fix doc deploy #1320

Merged
merged 13 commits into from
May 3, 2017
Merged

fix doc deploy #1320

merged 13 commits into from
May 3, 2017

Conversation

kain88-de
Copy link
Member

@kain88-de kain88-de commented Apr 26, 2017 •
edited
Loading

this also includes a short test of the if structure to see if it works in the PR
already.

Fixes #1316

Changes made in this Pull Request:

  • fix how to check for deployment of docs.

PR Checklist

- [ ] Tests?

  • Docs?
    - [ ] CHANGELOG updated?
  • Issue raised/referenced?

@kain88-de
fix doc deploy
2f796bb 
this also includes a short test of the if structure to see if it works in the PR
already.
@kain88-de kain88-de requested a review from orbeckst April 26, 2017 08:02
orbeckst
orbeckst requested changes Apr 26, 2017
View reviewed changes
Copy link
Member

@orbeckst orbeckst left a comment

Choose a reason for hiding this comment

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

No idea why this script does not understand test: it is a common Linux command.

But fix the bash syntax errors, check that it passes, and remove the dummy message.

.travis.yml Outdated
@@ -89,13 +89,17 @@ install:
script:
- cd ${TRAVIS_BUILD_DIR}
- if [[ $TRAVIS_OS_NAME == 'osx' ]]; then ulimit -S -n 2048; fi
- if [[ ${TRAVIS_PULL_REQUEST} == "true" && \
Copy link
Member

Choose a reason for hiding this comment

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

Do you want to keep this?? I assume it should be removed before merging.

Copy link
Member Author

Choose a reason for hiding this comment

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

I wanted to check my code as best as I could before we merge this to master to make sure it works. So yeah this will be removed before the merge

.travis.yml Outdated
@@ -89,13 +89,17 @@ install:
script:
- cd ${TRAVIS_BUILD_DIR}
- if [[ $TRAVIS_OS_NAME == 'osx' ]]; then ulimit -S -n 2048; fi
- if [[ ${TRAVIS_PULL_REQUEST} == "true" && \
${BUILD_DOCS} == "true"]]; then \
Copy link
Member

Choose a reason for hiding this comment

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

Syntax error: you need a space between "true" and ]].

Travis completely fails on this at the moment.

.travis.yml Outdated
bash ${TRAVIS_BUILD_DIR}/maintainer/deploy_docs.sh;
- if [[ ${TRAVIS_PULL_REQUEST} == "false" && \
${TRAVIS_BRANCH} == ${GH_DOC_BRANCH} && \
${BUILD_DOCS} == "true"]]; then \
Copy link
Member

Choose a reason for hiding this comment

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

Syntax error. See above.

@orbeckst
Copy link
Member

I quickly fixed the bash syntax errors using GitHub edit. Let's see what travis says.

@kain88-de
Copy link
Member Author

@orbeckst seems like you also didn't fix all errors.

@orbeckst
Copy link
Member

Not sure what the issue is

$ if [[ ${TRAVIS_PULL_REQUEST} == "true" && \ ${BUILD_DOCS} == "true" ]]; then \ echo "Yeah it works this should fix deployment"; \ fi

/home/travis/.travis/job_stages: eval: line 54: syntax error: unexpected end of file

Any ideas?

@orbeckst
fix syntax errors in travis.yml
b04768d 
Need yaml continuation blocks for multiline bash.
@orbeckst
Copy link
Member

I think we needed yaml continuation blocks

- |
  if ... stuff \
  more stude

... let's see if this did it.

@orbeckst
Copy link
Member

I don't think that the "Yeah it works" was actually echoed: https://travis-ci.org/MDAnalysis/mdanalysis/jobs/226159706#L658

This is the Travis task that has BUILD_DOCS=true. Am I missing something here??

@kain88-de
Copy link
Member Author

I can't see it either.

@kain88-de
Copy link
Member Author

This PR #1284 changed the deploy section of the travis config. I'll try using the old version again.

@kain88-de
Copy link
Member Author

OK I really broke everything again now. Strange

@kain88-de
Copy link
Member Author

OK the problem is that TRAVIS_PULL_REQUEST doesn't contain what we thought it would.

$ echo $TRAVIS_PULL_REQUEST

1320

@orbeckst
Copy link
Member

That's good to know!

@kain88-de
Copy link
Member Author

@orbeckst the solution i arrived at now works as far as I can tell. You can merge/squash this if you are ok with it.

@orbeckst orbeckst merged commit bb9bb57 into develop May 3, 2017
@orbeckst orbeckst deleted the test-test-command branch May 3, 2017 19:11
@orbeckst
Copy link
Member

orbeckst commented May 3, 2017 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@orbeckst orbeckst orbeckst approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

devdocs deployment broken
2 participants
@kain88-de @orbeckst