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

Remove isolated option while adding sys.path #335

Merged
merged 5 commits into from
Oct 6, 2023

Conversation

shatakshiiii
Copy link
Contributor

@shatakshiiii shatakshiiii commented Oct 3, 2023

This PR validates the usage of isolated while adding sys.path to collections path.

Closes: #324

@shatakshiiii shatakshiiii added the bug Something isn't working label Oct 3, 2023
@shatakshiiii shatakshiiii self-assigned this Oct 3, 2023
@shatakshiiii shatakshiiii temporarily deployed to ack October 3, 2023 12:21 — with GitHub Actions Inactive
@shatakshiiii shatakshiiii temporarily deployed to ack October 3, 2023 12:21 — with GitHub Actions Inactive
@shatakshiiii shatakshiiii temporarily deployed to ack October 4, 2023 04:52 — with GitHub Actions Inactive
@shatakshiiii shatakshiiii changed the title Validate isolated option Remove isolated option while adding sys.path Oct 4, 2023
@shatakshiiii shatakshiiii temporarily deployed to ack October 4, 2023 05:02 — with GitHub Actions Inactive
@shatakshiiii shatakshiiii temporarily deployed to ack October 4, 2023 05:08 — with GitHub Actions Inactive
@shatakshiiii shatakshiiii marked this pull request as ready for review October 4, 2023 05:14
@shatakshiiii shatakshiiii requested a review from a team as a code owner October 4, 2023 05:14
Copy link
Member

@ssbarnea ssbarnea left a comment

Choose a reason for hiding this comment

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

We do not want to lower the code coverage, removing of the test is a very bad idea and coverage was there to remind us not to make this mistake.

The correct solution for this is to modify the tests to check for the new behavior instead of removing them.

@shatakshiiii shatakshiiii temporarily deployed to ack October 4, 2023 15:12 — with GitHub Actions Inactive
@shatakshiiii shatakshiiii temporarily deployed to ack October 4, 2023 15:22 — with GitHub Actions Inactive
@cidrblock
Copy link
Collaborator

Removing the conditional allows the number of tests to be dropped. As time permits, please add a test or 2 in a seperate PR to return coverage to it's previous value or higher.

@shatakshiiii
Copy link
Contributor Author

shatakshiiii commented Oct 5, 2023

@ssbarnea ^^^ Also I realized that the changes in this PR is not responsible for lowering the code coverage, as I can see same codecov failure in other open PRs as well. And dropping some unnecessary tests is suppose to happen as per these code changes.

@shatakshiiii shatakshiiii temporarily deployed to ack October 6, 2023 07:41 — with GitHub Actions Inactive
@audgirka audgirka requested a review from ssbarnea October 6, 2023 13:08
@audgirka audgirka merged commit 7ae09fc into ansible:main Oct 6, 2023
20 checks passed
@shatakshiiii shatakshiiii deleted the sys_path branch October 6, 2023 14:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Addition of site packages directories via sys.path limited to isolated
4 participants