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

airbyte-ci: allow empty pull request number for migrate-base-image cmd #36220

Merged
merged 6 commits into from
Mar 24, 2024

Conversation

natikgadzhi
Copy link
Contributor

Sometimes you don't yet have the PR, so supplying the PR number doesn't make sense in certain cases. This PR would allow omitting changelog entry, folks would have to do that manually. Happy to add test cases!

@natikgadzhi natikgadzhi requested a review from a team as a code owner March 17, 2024 05:30
Copy link

vercel bot commented Mar 17, 2024

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

1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
airbyte-docs ⬜️ Ignored (Inspect) Visit Preview Mar 24, 2024 2:30am

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'm in for this change - added minor suggestions. I'm wondering if we should use a placeholder for PR number instead of not writing the entry at all.
The missing changelog entry will get caught by CI so I don't really mind the final behavior you'll pick.
Would you consider making the same change to the bump_version command which has the same annoying requirement for a PR number?

And please don't forget to bump the version in pyproject.toml and add a changelog entry in README :) (sorry, no autobump for this package)

@natikgadzhi
Copy link
Contributor Author

oh.. Running local formatting requires docker! 🙈

@natikgadzhi
Copy link
Contributor Author

@alafanechere please teach me to run formatters locally and let's make sure it's in the root-level CONTRIBUTING or something. I just ran poetry run isort something something at the root of the repo and it edited a bunch of files. Weird.

Anyway, done with feedback here.

@alafanechere
Copy link
Contributor

@alafanechere please teach me to run formatters locally and let's make sure it's in the root-level CONTRIBUTING or something. I just ran poetry run isort something something at the root of the repo and it edited a bunch of files. Weird.

Anyway, done with feedback here.

It is documented here https://docs.airbyte.com/contributing-to-airbyte/resources/code-formatting

@alafanechere
Copy link
Contributor

@natikgadzhi thanks for the changes. You might also want to update the documentation of the command you changed

@natikgadzhi
Copy link
Contributor Author

Fixed version, fixed readme, and fixed a lint error.

@natikgadzhi natikgadzhi enabled auto-merge (squash) March 24, 2024 02:30
@natikgadzhi natikgadzhi merged commit cc388fc into master Mar 24, 2024
32 checks passed
@natikgadzhi natikgadzhi deleted the ng/airbyte-ci/base-image branch March 24, 2024 02:50
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.

None yet

2 participants