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

Updates publish to USES_ENVIRONMENTS #17313

Merged
merged 4 commits into from
Oct 24, 2022

Conversation

chrisjrn
Copy link
Contributor

@chrisjrn chrisjrn commented Oct 21, 2022

This migrates publish to create the BuiltPackage asset in a correct environment, and run the publish executable in the local environment.

Also adds a helper rule, EnvironmentAwarePackageRequest, which consolidates the EnvironmentName request and BuiltPackage request, since BuiltPackage is requested all over the place.

See #17129.

@chrisjrn chrisjrn force-pushed the chrisjrn/17129-publish branch from b2c1488 to 3e8587e Compare October 21, 2022 19:19
@chrisjrn chrisjrn changed the title Chrisjrn/17129 publish Updates publish to USES_ENVIRONMENTS Oct 21, 2022
@chrisjrn chrisjrn added the category:internal CI, fixes for not-yet-released features, etc. label Oct 21, 2022
@chrisjrn chrisjrn marked this pull request as ready for review October 21, 2022 19:24
Comment on lines +247 to +250
res = await Effect(
InteractiveProcessResult,
{pub.process: InteractiveProcess, local_environment.val: EnvironmentName},
)
Copy link
Member

@stuhood stuhood Oct 21, 2022

Choose a reason for hiding this comment

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

It's likely that some publish implementations will need to pin the construction of the actual publish command to use back to the local environment... because otherwise they might consume environment variables / etc to construct it "for" a remote environment?

i.e.: Do packaging/etc in some environment, but then construct a publish command for the local environment.

I'm not sure if that suggests that they should each need to figure out their own environment internally instead... but ... maybe?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Isn't that what's going on here? The packaging request is using the environment that's needed to successfully construct the package, but the publish command is run locally.

packages = await MultiGet(
Get(BuiltPackage, PackageFieldSet, field_set) for field_set in request.package_field_sets
Get(BuiltPackage, EnvironmentAwarePackageRequest(field_set))
for field_set in request.package_field_sets
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@stuhood this is where the package operation is potentially performed in a different environment.

{
field_set._request(packages): PublishRequest,
local_environment.val: EnvironmentName,
},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@stuhood This is where the publish processes are constructed, pinned to the local environment.

Copy link
Member

@stuhood stuhood Oct 24, 2022

Choose a reason for hiding this comment

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

Yea, you're right! I forgot that PublishProcesses doesn't actually do any of the building, and that that happens via BuiltPackage.

@chrisjrn chrisjrn requested a review from stuhood October 24, 2022 15:37
Copy link
Member

@stuhood stuhood left a comment

Choose a reason for hiding this comment

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

Glad that this was already factored in a way that could be run in split environments like this! Very cool. Thanks to @kaos and @xlevus for the original publish implementation.

{
field_set._request(packages): PublishRequest,
local_environment.val: EnvironmentName,
},
Copy link
Member

@stuhood stuhood Oct 24, 2022

Choose a reason for hiding this comment

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

Yea, you're right! I forgot that PublishProcesses doesn't actually do any of the building, and that that happens via BuiltPackage.

@chrisjrn chrisjrn merged commit ab8202e into pantsbuild:main Oct 24, 2022
@chrisjrn chrisjrn deleted the chrisjrn/17129-publish branch October 24, 2022 20:03
@stuhood stuhood mentioned this pull request Oct 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:internal CI, fixes for not-yet-released features, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants