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

aws-elasticbeanstalk: Switch to Python 3 and improve the test slightly #47369

Closed
wants to merge 2 commits into from

Conversation

issyl0
Copy link
Member

@issyl0 issyl0 commented Dec 1, 2019

  • Have you followed the guidelines for contributing?
  • Have you checked that there aren't other open pull requests for the same formula update/change?
  • Have you built your formula locally with brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
  • Is your test running fine brew test <formula>, where <formula> is the name of the formula you're submitting?
  • Does your build pass brew audit --strict <formula> (after doing brew install <formula>)?

Part of #47050. See individual commits for more detail.

@Bo98 Bo98 added the python Python use is a significant feature of the PR or issue label Dec 1, 2019
@issyl0 issyl0 force-pushed the aws-elasticbeanstalk-python3 branch from c4602cc to 449330f Compare December 1, 2019 19:02
@Bo98
Copy link
Member

Bo98 commented Dec 1, 2019

There's a few resources for 2.7 compat that could be removed and may be causing problems with the build like backports.ssl_match_hostname and enum34.

@issyl0 issyl0 force-pushed the aws-elasticbeanstalk-python3 branch from 449330f to ff87de8 Compare December 1, 2019 19:54
@issyl0 issyl0 self-assigned this Dec 1, 2019
@issyl0 issyl0 added the test failure CI fails while running the test-do block label Dec 1, 2019
@issyl0
Copy link
Member Author

issyl0 commented Dec 2, 2019

If anyone has a clue as to why the test is failing, I'd appreciate a pointer. 😳 I've locally tried every permutation of \n, 2>&1 and .strip as in other shell_output examples.

- This actually exercises some of the functionality of the software:
  finding the specified profile's credentials.
@issyl0 issyl0 force-pushed the aws-elasticbeanstalk-python3 branch from ff87de8 to 01a6630 Compare December 3, 2019 10:32
@issyl0
Copy link
Member Author

issyl0 commented Dec 3, 2019

Thanks to @SMillerDev for helping with the test. It was the one thing I hadn't tried to fix it. 😬

@issyl0 issyl0 added ready to merge PR can be merged once CI is green and removed test failure CI fails while running the test-do block ready to merge PR can be merged once CI is green labels Dec 3, 2019
@issyl0
Copy link
Member Author

issyl0 commented Dec 3, 2019

@Bo98 I probably shouldn't stick a "ready to merge" label on this, should I? Would you be able to give it a review first? CI is green now, though.

@SMillerDev
Copy link
Member

You're as much a maintainer as the rest of us.

@SMillerDev SMillerDev added the ready to merge PR can be merged once CI is green label Dec 3, 2019
Copy link
Member

@Bo98 Bo98 left a comment

Choose a reason for hiding this comment

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

If it's green I'm happy with that.

This should be pretty trivial to migrate to 3.8 too since it depends on nothing else but Python and nothing else depends on it. I'll make sure it is noted in #47274.

@issyl0
Copy link
Member Author

issyl0 commented Dec 3, 2019

Thanks, @Bo98! I'm still pretty unconfident with the workflows in this repo - can I merge this as is via the GitHub UI or does it need more things?

@Bo98
Copy link
Member

Bo98 commented Dec 3, 2019

This one should be merged via brew pull --bottle. I’ll do it in a few hours if someone else hasn’t.

@SMillerDev
Copy link
Member

That's not needed, no bottles are build.

@Bo98
Copy link
Member

Bo98 commented Dec 3, 2019

But there is a bottle block in this formula?

@Bo98
Copy link
Member

Bo98 commented Dec 3, 2019

Actually this should also have a revision bump - I forgot about that.

@SMillerDev
Copy link
Member

No version change = no uploaded bottle changes

@chenrui333 chenrui333 closed this in 7d05ab3 Dec 4, 2019
@Bo98
Copy link
Member

Bo98 commented Dec 4, 2019

Yes, but there should be a revision change as we changed between Python 2 and 3. We create a virtualenv in the bottles.

@chenrui333
Copy link
Member

Oh, I just saw the ready to merge tag and merged the PR, is there anything that I should be concerned about? :)

@Bo98
Copy link
Member

Bo98 commented Dec 4, 2019

I think there's a disagreement on whether this should have had a revision bump. Perhaps best to propose the question in a separate PR.

@chenrui333
Copy link
Member

I think I did republish the bottle files, not sure if that helps.

==> Applying patch
Applying: aws-elasticbeanstalk: Switch to Python 3
Applying: aws-elasticbeanstalk: Improve the test slightly
Applying: aws-elasticbeanstalk: update 3.14.6 bottle.
==> Publishing on Bintray: aws-elasticbeanstalk 3.14.6
{"files":3}
==> Verifying bottles published on Bintray
Verifying bottle: aws-elasticbeanstalk-3.14.6.catalina.bottle.1.tar.gz
Waiting on Bintray.....
######################################################################## 100.0%

@chenrui333
Copy link
Member

The revision bump seems not quite needed (even though semantically it needs to happen)

@Bo98
Copy link
Member

Bo98 commented Dec 4, 2019

The problem is that people with existing installations will still have a dependency to python@2 - and we plan to remove python@2 at the end of the year.

@Bo98
Copy link
Member

Bo98 commented Dec 4, 2019

It seems like there's a 3.16.0 available now anyway so I suppose we could just try update the formula to that and then the case of whether it needs a revision bump no longer matters.

@issyl0 issyl0 deleted the aws-elasticbeanstalk-python3 branch December 4, 2019 16:55
@lock lock bot added the outdated PR was locked due to age label Jan 4, 2020
@lock lock bot locked as resolved and limited conversation to collaborators Jan 4, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age python Python use is a significant feature of the PR or issue ready to merge PR can be merged once CI is green
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants