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

Create apply_tags #998

Merged
merged 6 commits into from
Feb 8, 2020
Merged

Create apply_tags #998

merged 6 commits into from
Feb 8, 2020

Conversation

Nosferican
Copy link
Contributor

Draft!

Julia prints a bit more so we make it conformant with the rest.
Copy link
Member

@parente parente left a comment

Choose a reason for hiding this comment

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

@Nosferican Thanks for submitting this. The code you submitted looks right. We don't have any automated testing setup for the docker hub hooks. In lieu of that, could you try running the few new commands that fetch the Julia and R versions locally to ensure they return the expected values?

In addition to this script, you'll need to add a couple lines like these found in base-notebook/hooks/post_push to the top of datascience-notebook/hooks/post_push to source the apply_tags file:

# Apply and push all tags
source hooks/apply_tags
docker push $DOCKER_REPO

After confirming the commands work and adding those extra lines, I think this is good to merge and try.

Address code review.
- Added apply_tags to post_push
- Verified output for versions
@Nosferican
Copy link
Contributor Author

jovyan@381b43ea26a5:~$ python --version 2>&1 | awk '{print $2}'
3.7.6
jovyan@381b43ea26a5:~$ R --version |  sed -n 1p | awk '{print $3}'
3.6.1
jovyan@381b43ea26a5:~$ julia --version | awk '{print $3}'
1.3.1
jovyan@381b43ea26a5:~$ jupyter-notebook --version | tr -d '\r'
6.0.3
jovyan@381b43ea26a5:~$ jupyterhub --version | tr -d '\r'
1.1.0
jovyan@381b43ea26a5:~$ jupyter-lab --version | tr -d '\r'
1.2.5

Copy link
Member

@parente parente left a comment

Choose a reason for hiding this comment

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

@Nosferican I think there's a couple copy/paste errors in your last commit. If you accept those two tag name changes, I think we're good to merge.

datascience-notebook/hooks/apply_tags Outdated Show resolved Hide resolved
datascience-notebook/hooks/apply_tags Outdated Show resolved Hide resolved
Nosferican and others added 2 commits February 4, 2020 21:32
Co-Authored-By: Peter Parente <parente@gmail.com>
Co-Authored-By: Peter Parente <parente@gmail.com>
@parente
Copy link
Member

parente commented Feb 8, 2020

@Nosferican Thanks for sending this PR and working through improving it. 🍰

Let's merge it and see how it goes. If there are problems when Docker Hub attempts to build the image, we might need to iterate. I'll comment back here with the results since the build logs are, unfortunately, private.

@parente parente merged commit d2c9b7a into jupyter:master Feb 8, 2020
parente added a commit that referenced this pull request Feb 9, 2020
Follow on from #998
@parente
Copy link
Member

parente commented Feb 9, 2020

I fixed a small error missed during code review with a direct comment to master: 9f4983c#diff-57bd71ca260fa0afd2753f51e94b8420

thaume pushed a commit to metriqteam/docker-stacks that referenced this pull request Mar 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants