Remind users they should be setting a Python min version. #12287

RobbieMcKinstry · 2023-02-27T17:40:04Z

Description

This PR adds a log line during SDKGen if the user does not include a minimum
Python version. IMO, we should expect all Python packages to list a minimum
Python version to provide greater clarity to our users.

In the current version of this PR, this warning is printing during module
generating; we might prefer to ensure its only printed once per package instead
of once per module.

Additionally, this turns accessing the minimum Python version into a checked error from an unchecked error, increasing safety.

Fixes: #12286

Checklist

I have added tests that prove my fix is effective or that my feature works

I have run make changelog and committed the changelog/pending/<file> documenting my change

Yes, there are changes in this PR that warrants bumping the Pulumi Service API version

RobbieMcKinstry · 2023-02-27T17:40:16Z

Current dependencies on/for this PR:

master
- PR Remind users they should be setting a Python min version. #12287 👈
  - PR Remind the user to provide a License in Python codegen. #12288

This comment was auto-generated by Graphite.

pulumi-bot · 2023-02-27T17:40:47Z

Changelog

[uncommitted] (2023-03-30)

Features

[sdkgen/python] In codegen, use 3.7 as a default if not provided.
#12287

pkg/codegen/python/importer.go

pkg/codegen/python/gen.go

This commit introduces a log line to remind users they should be setting the Python minimum version in their schema. This refactor makes accessing the min version a checked error, increasing safety.

Prefer a top-level function over a method for minPythonVersion, because minPythonVersion is a validation step, and thus should be colocated with validation code. It's not a concern of the PackageInfo.

pkg/codegen/python/gen.go

Frassle · 2023-03-30T14:29:04Z

pkg/codegen/python/gen.go

@@ -2210,6 +2222,21 @@ func genPackageMetadata(
 	return w.String(), nil
 }

+// errNoMinimumPythonVersion is a non-fatal error indicating that the schema


Probably overkill for a != "" check.

Is this blocking feedback? I want a checked error because

we've already missed checking this field once

we're about to check it in a second place, with the advent of the pyproject.toml file.

That's kind of the whole point of this PR, really.

Fair enough, it doesn't feel very "Go-ish" but then Go is horrible so I won't block approval on this

Thanks! I had a similar discussion with @abhinav around the API limitations which (in my eyes) necessitate adding this fn on a public field.

12287: Remind users they should be setting a Python min version. r=RobbieMcKinstry a=RobbieMcKinstry  # Description This PR adds a log line during SDKGen if the user does not include a minimum Python version. IMO, we should expect all Python packages to list a minimum Python version to provide greater clarity to our users. In the current version of this PR, this warning is printing during module generating; we might prefer to ensure its only printed once per package instead of once per module. Additionally, this turns accessing the minimum Python version into a checked error from an unchecked error, increasing safety.  Fixes: #12286 ## Checklist  - [x] I have added tests that prove my fix is effective or that my feature works  - [x] I have run `make changelog` and committed the `changelog/pending/<file>` documenting my change  - [ ] Yes, there are changes in this PR that warrants bumping the Pulumi Service API version  Co-authored-by: Robbie McKinstry <robbie@pulumi.com>

RobbieMcKinstry · 2023-03-30T16:07:11Z

bors r+

bors · 2023-03-30T16:07:13Z

Already running a review

Frassle

I very much like that we now fill in a sensible default for setup.py files, good change.
I'm less sure about needing to tell everyone that they ought to set this version in their schemas. I'd consider setting this field to be a edge use case where users know better than our sdk generation because they're doing something custom.
I've said it elsewhere but to repeat it here, I think a good schema design would result in not having to set any language specific settings (we don't have that today, but we should be moving towards that, not away).

RobbieMcKinstry · 2023-03-30T16:18:20Z

I'm less sure about needing to tell everyone that they ought to set this version in their schemas.

Are you concerned about the log level? Suppose it was DEBUG instead of WARN.

I think if we're changing the value that users provide, we should emit a log somewhere telling them we're overriding their value.
How does that sound? That sounds like a reasonable in-between to me.

Frassle · 2023-03-30T16:24:36Z

I think if we're changing the value that users provide, we should emit a log somewhere telling them we're overriding their value.

I agree with that statement. But in this case we're not changing the value that users provide. Either they give us a min version and we stick that in setup.py, or they don't and we put in a sensible default. I don't think we need to log for every default, but if you think we should (and like I can see arguments for that) then that's fine but then we should be consistent and be logging for lots of other defaults we set as well not just this.

RobbieMcKinstry · 2023-03-30T16:29:43Z

That makes sense to me. My life is still made easier by this PR as long as we keep the checked error. I'm refactoring now.

bors · 2023-03-30T16:31:59Z

Canceled.

RobbieMcKinstry · 2023-03-30T16:36:59Z

bors r+

bors · 2023-03-30T17:05:39Z

Build succeeded:

bors-ok

RobbieMcKinstry requested review from abhinav and Frassle February 27, 2023 17:40

RobbieMcKinstry mentioned this pull request Feb 27, 2023

Remind the user to provide a License in Python codegen. #12288

Open

3 tasks

abhinav reviewed Feb 27, 2023

View reviewed changes

pkg/codegen/python/importer.go Outdated Show resolved Hide resolved

abhinav reviewed Feb 27, 2023

View reviewed changes

pkg/codegen/python/importer.go Outdated Show resolved Hide resolved

Frassle reviewed Feb 27, 2023

View reviewed changes

pkg/codegen/python/gen.go Outdated Show resolved Hide resolved

RobbieMcKinstry added 2 commits March 28, 2023 15:50

Remind users they should be setting a Python min version.

115a8a9

This commit introduces a log line to remind users they should be setting the Python minimum version in their schema. This refactor makes accessing the min version a checked error, increasing safety.

Use a default minimum_python_version if none if provided in the schema.

53d647c

RobbieMcKinstry force-pushed the mckinstry/refactor-py-metadata-gen branch from 657d217 to 53d647c Compare March 28, 2023 20:31

RobbieMcKinstry added 2 commits March 28, 2023 16:42

Make minPythonVersion a top-level function.

c05d802

Prefer a top-level function over a method for minPythonVersion, because minPythonVersion is a validation step, and thus should be colocated with validation code. It's not a concern of the PackageInfo.

Regen codegen snapshot data.

70ad059

RobbieMcKinstry requested review from abhinav and Frassle March 30, 2023 14:17

Frassle reviewed Mar 30, 2023

View reviewed changes

pkg/codegen/python/gen.go Outdated Show resolved Hide resolved

Frassle reviewed Mar 30, 2023

View reviewed changes

RobbieMcKinstry added 2 commits March 30, 2023 11:03

Pulumi supports Python >=3.7, change default value

9909964

Update changelog to reflect the change.

6c0fba2

Frassle reviewed Mar 30, 2023

View reviewed changes

Remove user-facing warning when min Python version not set.

c6fae07

RobbieMcKinstry requested a review from Frassle March 30, 2023 16:32

Frassle approved these changes Mar 30, 2023

View reviewed changes

bors bot merged commit 2972de9 into master Mar 30, 2023
47 of 52 checks passed

bors bot deleted the mckinstry/refactor-py-metadata-gen branch March 30, 2023 17:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remind users they should be setting a Python min version. #12287

Remind users they should be setting a Python min version. #12287

RobbieMcKinstry commented Feb 27, 2023 •

edited

RobbieMcKinstry commented Feb 27, 2023 •

edited

pulumi-bot commented Feb 27, 2023 •

edited

Frassle Mar 30, 2023

RobbieMcKinstry Mar 30, 2023

Frassle Mar 30, 2023

RobbieMcKinstry Mar 30, 2023

RobbieMcKinstry commented Mar 30, 2023

bors bot commented Mar 30, 2023

Frassle left a comment

RobbieMcKinstry commented Mar 30, 2023 •

edited by Frassle

Frassle commented Mar 30, 2023

RobbieMcKinstry commented Mar 30, 2023

bors bot commented Mar 30, 2023

RobbieMcKinstry commented Mar 30, 2023

bors bot commented Mar 30, 2023

Remind users they should be setting a Python min version. #12287

Remind users they should be setting a Python min version. #12287

Conversation

RobbieMcKinstry commented Feb 27, 2023 • edited

Description

Checklist

RobbieMcKinstry commented Feb 27, 2023 • edited

pulumi-bot commented Feb 27, 2023 • edited

Changelog

[uncommitted] (2023-03-30)

Features

Frassle Mar 30, 2023

Choose a reason for hiding this comment

RobbieMcKinstry Mar 30, 2023

Choose a reason for hiding this comment

Frassle Mar 30, 2023

Choose a reason for hiding this comment

RobbieMcKinstry Mar 30, 2023

Choose a reason for hiding this comment

RobbieMcKinstry commented Mar 30, 2023

bors bot commented Mar 30, 2023

Frassle left a comment

Choose a reason for hiding this comment

RobbieMcKinstry commented Mar 30, 2023 • edited by Frassle

Frassle commented Mar 30, 2023

RobbieMcKinstry commented Mar 30, 2023

bors bot commented Mar 30, 2023

RobbieMcKinstry commented Mar 30, 2023

bors bot commented Mar 30, 2023

RobbieMcKinstry commented Feb 27, 2023 •

edited

RobbieMcKinstry commented Feb 27, 2023 •

edited

pulumi-bot commented Feb 27, 2023 •

edited

RobbieMcKinstry commented Mar 30, 2023 •

edited by Frassle