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

Jump to bottom

Upgrade to Python3 #1214

Closed
wants to merge 3 commits into from
Closed

Upgrade to Python3 #1214

wants to merge 3 commits into from

Conversation

Taneb
Copy link
Contributor

@Taneb Taneb commented Dec 19, 2019 •
edited

I haven't tested this executes properly but it passes all the tests on Linux.

There's a note in nixops/diff.py suggesting some improvements to the type declarations possible in python3 which I haven't dealt with.

Nathan van Doorn and others added 3 commits December 19, 2019 18:48
Upgrade to Python3
39d6c31
@Taneb
Change unit test target
d8ff75e 
This test assumes peculiar ordering guarantees which have changed from
python2. This switches it to be the output from python3, but a proper
fix would either to test equality only up to permutation, or to fix and
guarantee an actual ordering.
@Taneb
Finalize python3 upgrade
1e88e28
@Taneb Taneb changed the title (WIP) Upgrade to Python3 Upgrade to Python3 Dec 19, 2019
@Taneb
Copy link
Contributor Author

Taneb commented Dec 27, 2019

The CI failure seems to be unrelated to my PR, it looks like the nix Travis config now creates /etc/nix for us so when we try to do it in the .travis.yml pre-step it fails.

@lheckemann
Copy link
Member

Did you use 2to3 for the syntactic/trivial changes? Why are they split into two commits?

As for the unit test target, I think it would be better to change assert_merge to compare set(functools.reduce(…)) and set(expect) instead, to better encode that we don't guarantee an order.

@Taneb
Copy link
Contributor Author

Taneb commented Jan 28, 2020

@lheckemann I did not use 2to3, the changes were manual. I agree with the solution to the test issue.

@grahamc
Copy link
Member

grahamc commented Feb 20, 2020

I'm seeing a lot of failures around .iteritems() which should be .items() instead. Why not use 2to3?.

grahamc
grahamc requested changes Feb 20, 2020
View reviewed changes
Copy link
Member

@grahamc grahamc left a comment

Choose a reason for hiding this comment

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

I'm +1 in principle, and excited to see this work -- and willing to use it!

I'm getting a lot of iteritems:

[nix-shell:~/projects/grahamc/packet-spot-community]$ nixops info
Traceback (most recent call last):
  File "/nix/store/piraap1wq1sbgyibyzf3hjy034264n44-nixops-1.8pre0_abcdef/bin/.nixops-wrapped", line 251, in <module>
    args.op(args)
  File "/nix/store/piraap1wq1sbgyibyzf3hjy034264n44-nixops-1.8pre0_abcdef/lib/python3.7/site-packages/nixops/script_defs.py", line 224, in op_info
    do_eval(depl)
  File "/nix/store/piraap1wq1sbgyibyzf3hjy034264n44-nixops-1.8pre0_abcdef/lib/python3.7/site-packages/nixops/script_defs.py", line 166, in do_eval
    depl.evaluate()
  File "/nix/store/piraap1wq1sbgyibyzf3hjy034264n44-nixops-1.8pre0_abcdef/lib/python3.7/site-packages/nixops/deployment.py", line 360, in evaluate
    self.evaluate_network()
  File "/nix/store/piraap1wq1sbgyibyzf3hjy034264n44-nixops-1.8pre0_abcdef/lib/python3.7/site-packages/nixops/deployment.py", line 352, in evaluate_network
    self.description = config.get("description", self.default_description)
  File "/nix/store/piraap1wq1sbgyibyzf3hjy034264n44-nixops-1.8pre0_abcdef/lib/python3.7/site-packages/nixops/util.py", line 260, in set
    else: self._set_attr(name, x)
  File "/nix/store/piraap1wq1sbgyibyzf3hjy034264n44-nixops-1.8pre0_abcdef/lib/python3.7/site-packages/nixops/deployment.py", line 142, in _set_attr
    self._set_attrs({name: value})
  File "/nix/store/piraap1wq1sbgyibyzf3hjy034264n44-nixops-1.8pre0_abcdef/lib/python3.7/site-packages/nixops/deployment.py", line 132, in _set_attrs
    for n, v in attrs.iteritems():
AttributeError: 'dict' object has no attribute 'iteritems'

@grahamc
Copy link
Member

grahamc commented Feb 20, 2020

It looks like the by-hand / partial conversion of deployment.py breaks 2to3.

@grahamc
Copy link
Member

grahamc commented Feb 20, 2020 •
edited

I did some more work on it here master...grahamc:python3 and make nixops info work, and nixops ssh-for-each work, but nixops deploy hangs forever with defunct ssh processes.

Update now it can do nixops deploy --force-reboot --exclude foo bar, but some problems around log line printing not printing enough \n's.

For example:

resource ‘s3-xlarge-x86-upstream-cpr’ is obsolete
resource ‘t3-small-x86-upstream-cpr’ is obsolete
resource ‘c3-small-x86-upstream-cpr’ is obsolete
resource ‘c3-medium-x86-upstream-cpr-b’ is obsolete
resource ‘c3-medium-x86-upstream-cpr’ is obsolete
< LONG DELAY, assuming it is asking questions... >
y
y
y
y
warning: are you sure you want to destroy Packet.Net machine ‘s3-xlarge-x86-upstream-cpr’? (y/N) warning: are you sure you want to destroy Packet.Net machine ‘c3-medium-x86-upstream-cpr-b’? (y/N) warning: are you sure you want to destroy Packet.Net machine ‘c3-small-x86-upstream-cpr’? (y/N) warning: are you sure you want to destroy Packet.Net machine ‘t3-small-x86-upstream-cpr’? (y/N) warning: are you sure you want to destroy Packet.Net machine ‘c3-medium-x86-upstream-cpr’? (y/N) c3-medium-x86-upstream-cpr-b> destroying instance 6777dadd-3e70-45d5-96b3-449b8a5d97fc
c3-small-x86-upstream-cpr...> destroying instance 6931fe78-e269-4a01-850e-9e9282a70ccd
t3-small-x86-upstream-cpr...> destroying instance 35be9080-2587-4d5e-8d80-39744db3ebe6
c3-medium-x86-upstream-cpr..> destroying instance 3d02b1da-3e05-4030-a819-15f21273fa32
ybuilding all machine configurations...

Update now it can make new systems with the nixops-packet backend but the "Waiting for SSH ......" periods don't print one at a time

c1-small-x86-upstream-cpr.> 147.75.71.69
 c1-small-x86-upstream-cpr.> waiting for SSH....................................................................................................................................................................
c1-small-x86-upstream-cpr.> Warning: Permanently added '147.75.71.69' (ED25519) to the list of known hosts.
c1-small-x86-upstream-cpr.> System provisioning file captured: {

Update: the logger correctly handles the "waiting for SSH ...." printing.

Update: this works correctly:

resource ‘c1-small-x86-upstream-cpr’ is obsolete
warning: are you sure you want to destroy Packet.Net machine ‘c1-small-x86-upstream-cpr’? (y/N) y
c1-small-x86-upstream-cpr.> destroying instance 01e881c7-1ad0-4979-a005-049456d7c5fd

@grahamc grahamc mentioned this pull request Feb 21, 2020
Closed
@grahamc grahamc closed this Mar 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@grahamc grahamc grahamc requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@Taneb @lheckemann @grahamc