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

Add conformance for rootDirectory pcl function #466

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

brandonpollack23
Copy link
Contributor

@brandonpollack23 brandonpollack23 commented Feb 14, 2025

Add conformance for projectRoot pcl function

Part of pulumi/pulumi#18565
Blocked by pulumi/pulumi#18595

Changes required to get project root in dotnet and have conformance
tests pass.

@brandonpollack23 brandonpollack23 force-pushed the bpollack/projectrootconformance branch 6 times, most recently from 8d0797e to 70227ac Compare February 18, 2025 09:54
@brandonpollack23 brandonpollack23 changed the title Add conformance for projectRoot pcl function Add conformance for rootDirectory pcl function Feb 18, 2025
@brandonpollack23 brandonpollack23 force-pushed the bpollack/projectrootconformance branch 2 times, most recently from e758114 to 96e3aa7 Compare February 20, 2025 05:31
@brandonpollack23 brandonpollack23 changed the base branch from main to bpollack/gomod_update February 20, 2025 05:32
@brandonpollack23 brandonpollack23 marked this pull request as ready for review February 20, 2025 05:33
@brandonpollack23 brandonpollack23 requested a review from a team as a code owner February 20, 2025 05:33
@brandonpollack23 brandonpollack23 force-pushed the bpollack/projectrootconformance branch 2 times, most recently from 11c9fda to c9e0260 Compare February 20, 2025 10:00
@brandonpollack23 brandonpollack23 force-pushed the bpollack/gomod_update branch 2 times, most recently from d9c4a33 to fd16e43 Compare February 20, 2025 10:21
@brandonpollack23 brandonpollack23 force-pushed the bpollack/projectrootconformance branch from c9e0260 to 0b45515 Compare February 20, 2025 10:21
Base automatically changed from bpollack/gomod_update to main February 20, 2025 11:18
@brandonpollack23
Copy link
Contributor Author

This should be good once blocking PR is done

@brandonpollack23 brandonpollack23 force-pushed the bpollack/projectrootconformance branch from 0b45515 to ec84337 Compare February 25, 2025 02:01
@brandonpollack23
Copy link
Contributor Author

blocking PR is approved, just running tests so this should be good to review

github-merge-queue bot pushed a commit that referenced this pull request Feb 27, 2025
Update `pulumi` submodule to the v3.153.0 tag. This brings in two new
conformance tests.

- `l1-builtin-project-root` support is being added in #466 
- `l2-component-property-deps` is blocked on .NET programgen support for
`Call` (and #488)

Somewhat suspicious that a couple generated SDKs changed (I ran the
conformance tests with `PULUMI_ACCEPT=true`). Are we not snapshot
comparing those?
@brandonpollack23 brandonpollack23 force-pushed the bpollack/projectrootconformance branch from ec84337 to cc541d6 Compare March 3, 2025 08:26

/// <summary>
/// The current running deployment instance. This is only available from inside the function
/// passed to <see cref="Deployment.RunAsync(Action)"/> (or its overloads).
/// </summary>
public static DeploymentInstance Instance
{
get => _instance.Value ?? throw new InvalidOperationException("Trying to acquire Deployment.Instance before 'Run' was called.");
get =>
Copy link
Member

Choose a reason for hiding this comment

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

Don't reformat whitespace everywhere, just makes the diff noisy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that shoudl be better, wasn't aware I had a strict formatter turned on

I also added a cache clear before testing to purge any old versions with
the same version number if present, as well as some docs describing this
issue which cost me an hour.

Part of #18565
Blocked by #18595

Changes required to get project root in dotnet and have conformance
tests pass.
@brandonpollack23 brandonpollack23 force-pushed the bpollack/projectrootconformance branch from cc541d6 to a8cf4fa Compare March 3, 2025 09:48
@@ -119,6 +121,7 @@ private Deployment(RunnerOptions? runnerOptions = null)
if (string.IsNullOrEmpty(monitor) ||
string.IsNullOrEmpty(engine) ||
string.IsNullOrEmpty(project) ||
string.IsNullOrEmpty(rootDirectory) ||
Copy link
Member

Choose a reason for hiding this comment

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

You can't add this here. If someone uses a new SDK with the old language host it will error out on this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okidoki

@@ -17,6 +17,7 @@ private Deployment(IDeploymentBuilder builder, InlineDeploymentSettings settings

_organizationName = settings.Organization ?? "organization";
_projectName = settings.Project;
_rootDirectory = "rootDirectory";
Copy link
Member

Choose a reason for hiding this comment

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

We should probably make root directory nulllable. Both running against the old engine, and running inline programs won't have a root directory.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -160,6 +163,7 @@ internal Deployment(IEngine engine, IMonitor monitor, TestOptions? options)
_isDryRun = options?.IsPreview ?? true;
_stackName = options?.StackName ?? "stack";
_projectName = options?.ProjectName ?? "project";
_rootDirectory = options?.RootDirectory ?? "rootDirectory";
Copy link
Member

Choose a reason for hiding this comment

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

Not sure this should default to anything? Like null is easier to match against than "rootDirectory'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suppose so, the others aren't nullable but it kinda makes me think they should be as well

Copy link
Member

Choose a reason for hiding this comment

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

Well the others always get set except org and "organisation" was I suppose a half sensible default choice for that, although in hindsight maybe that should have been nullable or throwing as well for old engines.

I think this probably could be set for inline programs as well (the autoapi does make a temp folder for them to run in). So it's just what to do for old engines, which I think has three choices:

  1. Be nullable, return null for old engines
  2. Don't be nullable and throw a runtime exception if running against an old engine. We do this for other things like transforms
  3. Don't be nullable and have a default value, but I'm not sure anything makes sense for that.

So actually thinking about it (and sorry to go back and forth on you like this) but maybe we should do option 2. Make the user API non-nullable, but internally track if it's actually set by the engine or not and if the user tries to access it throw an exception like we do if they try and register a stack transform and it isn't supported: throw new Exception("The Pulumi CLI does not support root directory. Please update the Pulumi CLI.");

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, agreed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok this is done now

@brandonpollack23 brandonpollack23 force-pushed the bpollack/projectrootconformance branch from 046af8d to c77d2d7 Compare March 4, 2025 14:14
@brandonpollack23 brandonpollack23 force-pushed the bpollack/projectrootconformance branch from c0fecc7 to 2d10121 Compare March 10, 2025 08:58
@brandonpollack23 brandonpollack23 force-pushed the bpollack/projectrootconformance branch from 38a47e3 to 89e5d82 Compare March 10, 2025 09:39
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.

2 participants