-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Remove all env #3212
Remove all env #3212
Conversation
Awesome — thanks @DustinMoriarty ! |
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.
In general looking good, minor suggestions.
Co-authored-by: Arun Babu Neelicattu <[email protected]>
Co-authored-by: Arun Babu Neelicattu <[email protected]>
Co-authored-by: Arun Babu Neelicattu <[email protected]>
This looks good to go right? |
Looks like it might need docs update as well. |
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.
Nice! This will also solve this issue, so I'm keen to get this merged 😄 👍🏻 (Once the docs have been added, of course)
@sinoroc : Which docs need to be updated? |
Ok. I see perhaps we could update managing-environments.md to show the new functionality. The old docs would not be wrong since the change is back compatible. However, we can streamline the documentation for env remove to show multiple lines removed at once. Am I understanding your suggestion right @sinoroc ? |
Yes. I had this in mind: but it actually is just a link to: So in the end, as you said, I guess docs should go to: At least that would be my recommendation (I am not a maintainer, so I do not have the last word on this). |
Got it. I'll see if I have some time to take a look this weekend. |
@sinoroc, @venaturum, @steveYeah or @abn : I just went to update the docs as discussed as well as update the branch to merge from latest upstream master. I am getting this failure with the FreeBSD build that does not look like it has anything to do with the PR. I think I had this problem before. It was odd because it was intermittent. Previously I would make some minor change, push and it would go away. |
Here is the error: FileNotFoundError: [Errno 2] No such file or directory: '/tmp/cirrus-ci-build/.tox/py/lib/python3.8/site-packages/ptyprocess-0.7.0.dist-info' |
Well I did do some additional changes to improve the CI help documentation. The FreeBSD check passed this time. Given the nature of that issue intermittently not being able to find a file I wonder if the Cirrus FreeBSD service is running low on storage. I guess all is well for this PR, but maybe something to look into with regards to the Cirrus FreeBSD check. |
I have seen multiple reports of CI failing on FreeBSD. I would not worry about this too much. Usually just re-triggering the build solves it. |
Sounds good. @abn or @steveYeah : could you please review? Is this ready to merge? |
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.
This has some conflicts -- LGTM once they're solved.
@neersighted : I merged from the upstream master and had no merge conflicts. It looks like it passes the pre-commit checks. Can you please approve the workflow pipeline to run? Thank you. |
@neersighted : It looks like this one passed all pipeline checks. I believe that all suggested changes are addressed. Can you please approve the merge? Thank you. |
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'd like a couple changes to the command descriptions, and I'm not 100% thrilled with the less-than-DRY approach. However, the code is simple to understand. Slight refactoring in env.py
is needed to DRY this up -- that can be left as a future task.
Co-authored-by: Bjorn Neergaard <[email protected]>
Co-authored-by: Bjorn Neergaard <[email protected]>
Co-authored-by: Bjorn Neergaard <[email protected]>
Co-authored-by: Bjorn Neergaard <[email protected]>
@neersighted : All suggestions should be resolved now. Thank you for the suggestions. |
Awesome! I'm going to squash your changes, but keep in mind Github lets you batch suggested edits into one commit. I'd have to squash here anyway, but it's worth keeping in mind if you do like using the web interface. |
…#3212) Co-authored-by: Arun Babu Neelicattu <[email protected]> Co-authored-by: Bjorn Neergaard <[email protected]>
…#3212) Co-authored-by: Arun Babu Neelicattu <[email protected]> Co-authored-by: Bjorn Neergaard <[email protected]>
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Pull Request Check List
Resolves: #3208
Resolves: #1884