-
Notifications
You must be signed in to change notification settings - Fork 167
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
ansible: add Pipfile and make python3 compatible #1399
Conversation
Ref: #1389 |
P.S. workflow is:
> cd ansible
> pipenv install
> pipenv shell
(venv)> ansible-playbook ...
or
> pipenv run ansible-playbook ... |
@refack Would you be able to update |
I don't understand any of this, I just know ansible is broken for me on my mac does it have to be so heavy-weight? switching to |
Rebase? |
Code has been reworked and rebased.
|
infra-* | ||
!node-www.tmpl |
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.
what's this for?
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.
We're tracking this file https://github.com/nodejs/build/blob/master/setup/www/host_vars/node-www.tmpl
(keeping it ignored, make my IDE angry)
So, with this we'd need to execute ansible from inside Can you explain the logic of this for me and what the dependencies are for? |
And: does this crate necessary dependencies? Will running the playbooks directly with |
It's an alternative approach.
It's still possible to use global dependencies just as before. Or we can use
example: |
OK, as long as it's documented I could live with it. I'll try it out soon on some of my known-breaking Python 3 Ansible tasks. |
this worked on a freebsd and ubuntu 14.04 reprovision but the problem I have with ccache jinja2 scripting persists:
|
You could try putting | list | in between the two calls to map(). Python 3 often creates generators instead of lists so perhaps forcing the results of the first map() call into a list will placate Jinja2. Update: Did not work at #1674 :-( |
👍 make sense. anyway. |
'macstadium', 'marist', 'mininodes', 'msft', 'osuosl', | ||
'rackspace', 'requireio', 'scaleway', 'softlayer', 'voxer', | ||
'packetnet', 'nearform') | ||
# taken from nodejs/node.git: ./configure |
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 they are in sync anymore. https://github.com/nodejs/node/blob/ed2c6965d2f901f3c786f9d24bcd57b2cd523611/configure.py#L48-L49
valid_arch = ('arm', 'arm64', 'ia32', 'ppc',
'ppc64', 'x32','x64', 'x86', 'x86_64', 's390', 's390x')
parser.add_argument('--host', action='store') | ||
args = parser.parse_args() | ||
|
||
# parser = argparse.ArgumentParser() |
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 checking in commented code?
@@ -6,9 +6,9 @@ | |||
with open('out/index.csv') as index: | |||
index_csv = filter(lambda line: line, index.read().split('\n')) | |||
|
|||
# noqa |
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 comment incomplete?
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.
LGTM: This flake8 linter directive will silence the linter as discussed at
http://flake8.pycqa.org/en/3.7.7/user/violations.html#in-line-ignoring-errors
@refack Can you please go through the comments (esp. #1399 (comment)) and make any required updates? It would be a big help to have pipenv in place. pipenv has the effect of running Python in its own virtualenv on the user's machine which means that the Python version is more predictable and that no outside dependencies are installed. |
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.
Let's move forward with this to ease the transition to Python 3.
@cclauss Unfortunately, @refack has not been active lately. My hope is he's just taking a break from Node.js stuff and hasn't left entirely, but either way, it would probably be best for someone to take this over and finish it. Are you up for doing that? |
* add Pipfile * de-lint with `flake8`
b17f2fa
to
e7ee43c
Compare
My sense is that this can be closed unless @refack has objections. I like the idea of automating the creation of virtualenvs for our builds to take place in and pipenv would deliver that. However @sam-github and I agreed that there might be compatibility issues on non-mainstream Win/Mac/Linux so we should delay that experimentation until after we have Python 3 working. |
In that case, I"m going to close this, but @refack or anyone else should feel absolutely free to re-open it if they think that's the wrong move. |
Pipfile
to describe dependenciesPEP8
compliant and add.flake8