-
Notifications
You must be signed in to change notification settings - Fork 47
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
Try deploying gh-pages only if change to docs dir #704
Conversation
@ggouaillardet Does this look right to you? |
Fixes #703 |
Signed-off-by: Ralph Castain <rhc@open-mpi.org>
scripts/deploy.sh
Outdated
@@ -0,0 +1,14 @@ | |||
#!/bin/bash | |||
set -ev | |||
testchange=`git diff --exit-code -- docs > /dev/null` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the rationale that if there's an actual diff in the docs
directory, then something meaningful changed, and therefore it's worth going through with the deploy?
If so, this might be worth a comment, because Future Jeff / Future Ralph / Future Someone might wonder why this two-step process is here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per @ribab:
Only run gh-pages travisci IF something in mtt/docs changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can add a comment - my question to @ggouaillardet was: does this work the way I think it should?
scripts/deploy.sh
Outdated
#!/bin/bash | ||
set -ev | ||
testchange=`git diff --exit-code -- docs > /dev/null` | ||
if [$testchange]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think $testchange will be loaded with 0 or 1.
I think you actually want:
# --quiet inhibits "git diff" output and implies --exit-code
git diff --quiet -- docs
if test $? -eq 1; then
..but then again, I'm slightly confused because this does not appear to be a pure Bourne shell script...?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested the command and it worked exactly as desired - I get a 0 or a 1
FWIW: the logic of that script is extracted directly from the Travis documentation 😄 |
scripts/deploy.sh
Outdated
#!/bin/bash | ||
set -ev | ||
testchange=`git diff --exit-code -- docs > /dev/null` | ||
if [$testchange]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the test be if [ $? -eq 0 ]
? $testchange
should always be empty.
And because of the set -e
we might never run the test if the previous git
command failed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay, I changed it - but then we have to also remove the "set -ev" line else we will exit if the command returns a non-zero status code, and Travis is adamant that we not call exit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW: my test showed that testchange was always either equal to 0 or 1, not empty
Signed-off-by: Ralph Castain <rhc@open-mpi.org>
scripts/deploy.sh
Outdated
set -ev | ||
testchange=`git diff --exit-code -- docs > /dev/null` | ||
if [$testchange]; then | ||
deploy: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All I can think of is this is not bash shell (the name and syntax suggests this should be a bash shell).
I will double check the Travis syntax tomorrow.
Btw, what is the rationale for this PR ? Is this a simple optimization (e.g. it does not fix anything) to save Travis resources ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ggouaillardet I asked a question on the MTT devel list to see if we could not re-deploy every time there's a commit to MTT. It causes a git push + email in the docs tree, and the only thing it contains is a timestamp update in the latex source. E.g.: 41bbb3c
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I'm not convinced this is actually worth all this angst, but was willing to spend five minutes on it just to keep the git repo from unnecessarily growing. If it goes back/forth much more, though, I'll drop it as not worth the time.
BTW, is the |
Danged if I know. @ggouaillardet Here is the link to the Travis doc: https://docs.travis-ci.com/user/customizing-the-build/#implementing-complex-build-steps |
Hah! Yeah, I agree it's not worth a huge amount of time. But... @ggouaillardet if you could spend 5 minutes on it tomorrow, that would then be a global total of 10 minutes of each of our time, and if that carries this over the finish line, #winning. 😉 |
@rhc54 the doc simply states a script can be executed. I did not read the script is supposed to output a Travis config that will be parsed dynamically. Also, from the deploy doc section
So at first glance, I do not think this looks right (we would have to modify the Travis provider in order to achieve the desired result). The right approach could be to manually push to the Makes sense ? |
I'm open to whatever solution (including doing nothing) someone wants to pursue 😄 |
@rhc54 Any reason why you closed this PR ? Right now, doing nothing is the best option. The rationale is the latex file contains some time stamps, so there is always something to push. (It looks like a latex bug/lack of feature, that is likely fixed in the latest version). I do not think the desired behavior can be achieved with some configuration only. |
I closed it because you indicated it was wrong, so no point in retaining it. If you believe you have a way of solving it, feel free to pursue it. Just don't burn a ton of time on it as it isn't something that critical. |
@ggouaillardet If you're interested... I think this PR had the right idea:
|
@rhc54 @jsquyres I think I now got the rationale right. The missing piece of the puzzle - from my point of view - is we need to (sort of) I assume the github oauth token is from Ralph's account, so I hardcoded |
I think, but could be wrong, that you won't need the I have no idea how to test it other than commit and see what happens. Note that we already lost some of the pages as reported in #705. No idea why. |
yep, it seems the user is sort of encoded in the oauth token. I will update the PR accordingly. |
Kewl - I'd say let's give it a try. If all works okay, we can then add the |
Signed-off-by: Ralph Castain rhc@open-mpi.org