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

#181 Update habushu container support to be more consistent across build platforms #182

Merged
merged 1 commit into from
Aug 1, 2024

Conversation

carter-cundiff
Copy link
Contributor

@carter-cundiff carter-cundiff commented Jul 31, 2024

Fixes #181

@carter-cundiff carter-cundiff force-pushed the 181-container-support-fix branch from 543c11f to 942f552 Compare July 31, 2024 18:35
arguments.add("config");
arguments.add("virtualenvs.in-project");
arguments.add("--local");
String currentInProjectSetting = poetryHelper.execute(arguments);
Copy link
Contributor

Choose a reason for hiding this comment

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

Q: This is just to ensure we've got the poetry.toml file sync'd in the case that the user has a pre-existing .venv folder, but the config hasn't been set in the toml file? (Perhaps in the case it was manually modified to remove the config?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct, see comment below

@ewilkins-csi
Copy link
Contributor

Update draft release notes if you have access. If not let me know and I can update them.

// update the poetry config to match the useInProjectVirtualEnvironment boolean
if (this.useInProjectVirtualEnvironment && Boolean.FALSE.equals(Boolean.valueOf(currentInProjectSetting))) {
getLog().info("Configuring Poetry to use an in-project virtual environment...");
configureVirtualEnvironmentsInProject(true);
Copy link
Contributor Author

@carter-cundiff carter-cundiff Jul 31, 2024

Choose a reason for hiding this comment

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

I: Previously we only updated the poetry config for virtualenvs.in-project when a .venv didn't exist. This will set the virtualenvs.in-project regardless of if a .venv exists, ensuring the poetry.toml always matches the value of useInProjectVirtualEnvironment.

Copy link
Contributor

Choose a reason for hiding this comment

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

We should test going both in-project versus default location and back for this. I recall that the original logic accounted for a challenge in that sequencing.

Copy link
Contributor Author

@carter-cundiff carter-cundiff Jul 31, 2024

Choose a reason for hiding this comment

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

Tested going back and forth between in project and external virtual environments without any issue. Both the current version (2.16.1) and the updated version (2.17.0-SNAPSHOT) showed the following behavior:

Case 1: external -> in project

  • External environment is deleted
  • In project environment is created and used
  • poetry.toml is updated with in-project = true

Case 2: in project -> external

  • In project environment is deleted
  • External environment is created and used
  • poetry.toml is updated with in-project = false

The only difference in behavior is if you have habushu.useInProjectVirtualEnvironment=true and don't have in-project = true in your poetry.toml but have an in project environment:

  • 2.17.0-SNAPSHOT will add in-project = true to your poetry.toml and use the in project environment
  • 2.16.1 will leave the poetry.toml unchanged and create an external environment, ignoring the in project environment

The current behavior of 2.16.1 seems like a bug to me that would be resolved with the 2.17.0-SNAPSHOT behavior.

@carter-cundiff carter-cundiff merged commit 59ee4f9 into dev Aug 1, 2024
7 checks passed
@carter-cundiff carter-cundiff deleted the 181-container-support-fix branch August 1, 2024 13:06
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.

Update habushu container support to be more consistent across build platforms
3 participants