-
Notifications
You must be signed in to change notification settings - Fork 3
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
Script to automate RTD config file update #59
Conversation
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.
This is so good.
I just have a couple of comments about making the process smoother in case there are issues with individual repos.
REPODIR="$WORKDIR/$repo" | ||
echo "Updating $repo" | ||
git clone git@github.com:mdolab/"$repo".git | ||
cd $REPODIR |
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 would have a safety check here to make sure that if a repo is missing from the list, we just move on to the next repo instead of exiting.
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.
Added some safeguards by checking exit conditions
echo "Updating $repo" | ||
git clone git@github.com:mdolab/"$repo".git | ||
cd $REPODIR | ||
git checkout -b $BRANCH_NAME |
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.
similar here, if for any reason we already have a branch with that name, I would add a check (or maybe a user prompt?) on whether to continue or not
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.
Added some safeguards by checking exit conditions
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.
Very nice, thanks for the fixes! I just have a final comment - maybe there's a function that has been left over there? I would still echo
a failure message from checkFailure
rtd/updateYaml.sh
Outdated
# Init array to track progress | ||
declare -A REPO_STATUS | ||
|
||
repoFailed () { |
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 this function being used?
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.
Good catch, I was going to use it, but decided not to in the end. Just moved the error echo instead. Should be good now.
If no one else has comments, I will merge this tomorrow morning. |
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.
Nice job, this looks very thourough
if [[ -z $MANUAL_REPO && -z $REPO_LIST ]]; then | ||
echo "Insufficient inputs, see usage:" | ||
echo "$USAGE" | ||
die |
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.
nice
Purpose
This PR adds an initial script that can be used to automate the update of RTD config files for a number of our repositories. The idea is to minimize the maintenance needed managing this particular file until some other solution, such as templating, is supported (see https://github.com/mdolab/private-docs/issues/298 for some discussion). If not, we could improve on this later by automatically generating the
yaml
file.Expected time until merged
No rush
Type of change
Testing
Checklist
flake8
andblack
to make sure the Python code adheres to PEP-8 and is consistently formattedfprettify
or C/C++ code withclang-format
as applicable