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

powershell: fix prepend for new variables and fix unsetenv for non-existing variables #1477

Conversation

maxnbk
Copy link
Contributor

@maxnbk maxnbk commented Apr 21, 2023

Closes #1476
Supercedes PR by @SitiSchu on #1434 (but includes his commit cherrypicked to maintain history/credit)

The test, and commits, also address the same issue but in the "unset" logic, because that too is a victim of this same issue, which is validated by the test, and was in fact the reason I came complaining to @instinct-vfx about not understanding something I was seeing, I just wasn't bright enough to understand it right away.

@maxnbk maxnbk requested review from nerdvegas and a team as code owners April 21, 2023 23:05
@maxnbk maxnbk force-pushed the 1476-addressing-missing-test-for-powershell-prependenv-unset branch from 4e0b7fd to e134aff Compare April 21, 2023 23:07
@JeanChristopheMorinPerso
Copy link
Member

@maxnbk FYI, since you are admin on the repo, you could have pushed your commit into @SitiSchu 's PR.

@maxnbk maxnbk force-pushed the 1476-addressing-missing-test-for-powershell-prependenv-unset branch 3 times, most recently from 90e75e5 to 3b9733c Compare April 21, 2023 23:17
@maxnbk
Copy link
Contributor Author

maxnbk commented Apr 21, 2023

@maxnbk FYI, since you are admin on the repo, you could have pushed your commit into @SitiSchu 's PR.

How? The branch exists on their fork.

@JeanChristopheMorinPerso
Copy link
Member

If the contributor checks the "Allow maintainers to edit PR", the contributor grants the admins/maintainers access to push to their branch in their fork. See https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork#enabling-repository-maintainer-permissions-on-existing-pull-requests.

On user-owned forks, if you want to allow anyone with push access to the upstream repository to make changes to your pull request, select Allow edits from maintainers.

@maxnbk
Copy link
Contributor Author

maxnbk commented Apr 22, 2023

If the contributor checks the "Allow maintainers to edit PR", the contributor grants the admins/maintainers access to push to their branch in their fork. See https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork#enabling-repository-maintainer-permissions-on-existing-pull-requests.

On user-owned forks, if you want to allow anyone with push access to the upstream repository to make changes to your pull request, select Allow edits from maintainers.

Gotcha, good to know.

Copy link
Member

@JeanChristopheMorinPerso JeanChristopheMorinPerso left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks @maxnbk and @SitiSchu !

@JeanChristopheMorinPerso JeanChristopheMorinPerso added this to the Next milestone May 5, 2023
@JeanChristopheMorinPerso JeanChristopheMorinPerso requested a review from a team May 27, 2023 21:12
@sonarcloud
Copy link

sonarcloud bot commented Jul 18, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@maxnbk maxnbk force-pushed the 1476-addressing-missing-test-for-powershell-prependenv-unset branch from 7ccc15d to 0087825 Compare September 6, 2023 20:21
@maxnbk maxnbk removed the request for review from nerdvegas September 6, 2023 20:22
@JeanChristopheMorinPerso JeanChristopheMorinPerso changed the title 1476 addressing missing test for powershell prependenv unset rex: fix prepend for new variables and fix unsetenv for non-existing variables Sep 6, 2023
@JeanChristopheMorinPerso JeanChristopheMorinPerso changed the title rex: fix prepend for new variables and fix unsetenv for non-existing variables powershell: fix prepend for new variables and fix unsetenv for non-existing variables Sep 6, 2023
@JeanChristopheMorinPerso JeanChristopheMorinPerso merged commit bb0d148 into AcademySoftwareFoundation:master Sep 9, 2023
32 checks passed
@JeanChristopheMorinPerso
Copy link
Member

Thanks @maxnbk and @SitiSchu !

Copy link
Contributor

@instinct-vfx instinct-vfx left a comment

Choose a reason for hiding this comment

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

LGTM looks good. Thanks @SitiSchu and @maxnbk

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

addressing missing test for powershell prependenv / unset
5 participants