-
Notifications
You must be signed in to change notification settings - Fork 0
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
Format change and misc edits #90
base: master
Are you sure you want to change the base?
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.
See my questions below.
apt: | ||
name: ['python3', 'python3-setuptools', 'python3-dev', 'python3-pip', 'sudo', 'npm', 'nodejs-legacy', 'git'] | ||
state: present | ||
update_cache: yes |
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.
Why is this using an array instead of the with_items it was previously using?
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.
Apparently with_items: has been deprecated but maybe I should be using a loop: instead?
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 found some docks on migrating from with_X to loop.
Sounds like you could just change the word with_items
to loop
in the original code.
--- | ||
# file: roles/jupyter/tasks/main.yml | ||
# requires python3 | ||
|
||
#################################### | ||
# Install base dependencies |
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.
Why is this file here? Doesn't git already have a backup of the previous version of this file?
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.
Out of habit... when I make changes to a file, I create a .bak and in this case, I would have deleted this file if I was certain that the install was successful. Me being overly cautious.
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.
You do not need to keep a copy of the original file. Since we are using a git repository you can get back to what the original file looked like. You can even see all the different versions of a file. For example here all versions of jupyterhub/tasks/main.yaml from this repo.
name: ['python37', 'python37-setuptools', 'python37-devel', 'python37-pip', 'sudo', 'npm', 'nodejs', 'git'] | ||
state: present | ||
update_cache: yes | ||
when: ansible_os_family == 'Debian' |
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.
Why is this Debian now instead of RedHat?
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 misunderstood the error output when I ran the playbook. I've reversed the edit.
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 see the updated change in github. You may need to commit your changes (once you are happy with them) and push to github. Something like git add jupyterhub/tasks/main.yaml
, then git commit
, then git push
- assuming you are still on this same branch.
Assuming in your code the command is yum
and the family is "RedHat": Is python37
a valid module name for CentOS 7?
when: spawner == 'docker' | ||
tags: jupyter-deps | ||
|
||
- name: install npm package configurable-http-proxy | ||
npm: name=configurable-http-proxy global=yes state=present | ||
when: spawner == 'docker' |
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.
Why are we limiting this just to docker? Aren't these needed for our non-docker use case?
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 should have gotten a better understanding of what -name: install npm package configurable-http-proxy did. I've removed the when: line.
@@ -274,7 +263,7 @@ | |||
c.BatchSpawnerBase.req_runtime = '{{ req_runtime }}' | |||
c.BatchSpawnerBase.req_memory = '{{ req_memory }}' | |||
c.BatchSpawnerBase.env_keep = ['LANG','JUPYTERHUB_API_TOKEN','JPY_API_TOKEN'] | |||
c.SlurmSpawner.batch_script = """#!/bin/bash | |||
c.SlurmSpawner.batch_script = ""#!/bin/bash |
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.
Why was the single quote removed here? Triple-quotes are a way to span strings across multiple lines in python.
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 was paying too much attention to the format color of the file in my editor. When I removed the extra quote, the formatting of the color showed that the issue was corrected. Note to self not to rely on an editor's color formatting in pointing out issues.
apt: | ||
name: ['python37', 'python37-setuptools', 'python37-devel', 'python37-pip', 'sudo', 'npm', 'nodejs', 'git'] |
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.
Why did this change from yum to apt?
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.
Testing and I forgot to change it back. Fixed.
@@ -321,7 +310,7 @@ | |||
when: ansible_os_family == 'RedHat' | |||
|
|||
- name: Ensure hostname is in /etc/hosts file | |||
lineinfile: dest=/etc/hosts regexp="{{ansible_hostname}}" line="{{ip}} {{ansible_hostname}}" | |||
lineinfile: dest=/etc/hosts regexp="{{ansible_hostname}}" line="{{ip}}" line="{{ansible_hostname}}" |
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 you can have multiple line
parameters, can you? And even if you can, the IP address and hostname need to be on the same line, so it's not clear what you mean to fix here. @gsteffen1921 can you explain?
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.
You can't have multiple line parameters. I've changed it back. I was experimenting with the formatting of the lineinfile: and failed to return it back to the original format.
Wasn't sure if...
lineinfile:
dest: /etc/hosts
regexp: "{{ansible_hostname}}"
line:"{{ip}} {{ansible_hostname}}"
would have been a better way to format.
when SyntaxError: Unexpected identifier running |
@gsteffen1921 Do you know if the following command installed this old version of nodejs or perhaps didn't run? gcb-ansible-roles/jupyterhub/tasks/main.yml Lines 24 to 34 in 7b97af3
There is some documentation on the nodejs website about installing from a package manager: https://nodejs.org/en/download/package-manager/ |
Results from the jupyterhub playbook tell me that nodejs-6.17.1 was being installed as part of the task. However, I don't know if nodejs was already installed prior to me running the playbook or being installed from prior attempts at running the playbook. "results": [ |
@gsteffen1921 if you read the documentation at nodejs, you will see that initializing the package repo determines the major version of nodejs, and that once one major version is installed, it will not be upgraded to a later major version. There may be Ansible roles for handling nodejs installation in a more configurable / Ansible type of way, you'd have to check in the Ansible galaxy. |
Changes made to task main.yml file installed jupyterhub but 'jupyterhub start' fails. Looks to be a python3.6 issue but I'm still checking into it. When you get a chance, can you see if anything glaring I missed could be the issue?