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

feat(bump-version): bump-version support for explicit version #42970

Conversation

bnchrch
Copy link
Contributor

@bnchrch bnchrch commented Aug 2, 2024

What

Updates bump_version to allow for explicit setting of a version

Why

This is to support the temporary decision to keep source-declarative-manifest and the cdk with the same version

In the long term we plan to add a better way to resolve CDK to SDM versions on the platform

Copy link

vercel bot commented Aug 2, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Skipped Deployment
Name Status Preview Comments Updated (UTC)
airbyte-docs ⬜️ Ignored (Inspect) Visit Preview Aug 9, 2024 3:50pm

Copy link
Contributor Author

bnchrch commented Aug 2, 2024

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @bnchrch and the rest of your teammates on Graphite Graphite

@bnchrch bnchrch marked this pull request as ready for review August 2, 2024 15:59
@bnchrch bnchrch requested a review from a team as a code owner August 2, 2024 15:59
@bnchrch bnchrch force-pushed the 08-02-feat_bump-version_bump-version_support_for_explicit_version branch from b06b405 to 2618320 Compare August 2, 2024 16:54
version_str = value.split("version:", 1)[1]
if semver.VersionInfo.is_valid(version_str):
return value
self.fail(f"{value} is not a valid bump type. Valid choices are {self.choices} or 'version:<semver>'.", param, ctx)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
self.fail(f"{value} is not a valid bump type. Valid choices are {self.choices} or 'version:<semver>'.", param, ctx)
self.fail(f"{value} is not a valid bump type. Valid choices are {self.choices} or 'version:<semver-version>'.", param, ctx)

return value
if value.startswith("version:"):
version_str = value.split("version:", 1)[1]
if semver.VersionInfo.is_valid(version_str):
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we care that this allows for e.g. dev versions, alpha versions etc.? My guess is probably not, since we don't intend it to be used much.

@bnchrch
Copy link
Contributor Author

bnchrch commented Aug 2, 2024

Thanks @erohmensing

Since we dont need this ASAP. Im going to wait to see if @alafanechere has some comments.

Copy link
Contributor

@bnchrch that's fine, we should just keep a sharp eye out for errors in that publish channel until this is merged :)

Copy link
Contributor

@natikgadzhi natikgadzhi left a comment

Choose a reason for hiding this comment

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

Haven't tested manually, but the code looks like it'd work.

Appease mypy gods:

pipelines/airbyte_ci/connectors/bump_version/commands.py:16: error: Function is missing a return type annotation  [no-untyped-def]
pipelines/airbyte_ci/connectors/bump_version/commands.py:16: note: Use "-> None" if function does not return a value
pipelines/airbyte_ci/connectors/bump_version/commands.py:19: error: Function is missing a type annotation  [no-untyped-def]
pipelines/airbyte_ci/connectors/bump_version/commands.py:28: error: Function is missing a type annotation  [no-untyped-def]
Found 3 errors in 1 file (checked 156 source files)

Copy link
Contributor

@alafanechere alafanechere left a comment

Choose a reason for hiding this comment

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

I like this: an elegant way to workaround your issue 👏

name = "bump-type"

def __init__(self):
self.choices = ["patch", "minor", "major"]
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: wondering if there could be an enum with these values we could pulled from the semver lib

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please update the bump-version doc?

@bnchrch bnchrch force-pushed the 08-02-feat_bump-version_bump-version_support_for_explicit_version branch 2 times, most recently from efccd5f to f2db0de Compare August 9, 2024 15:15
@bnchrch bnchrch force-pushed the 08-02-feat_bump-version_bump-version_support_for_explicit_version branch from f2db0de to 03df7ac Compare August 9, 2024 15:49
@bnchrch bnchrch merged commit eb74cba into master Aug 9, 2024
33 checks passed
Copy link
Contributor Author

bnchrch commented Aug 9, 2024

Merge activity

@bnchrch bnchrch deleted the 08-02-feat_bump-version_bump-version_support_for_explicit_version branch August 9, 2024 16:08
LouisAuneau pushed a commit to LouisAuneau/airbyte that referenced this pull request Aug 13, 2024
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.

4 participants