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

introduce ImageModifier #32876

Merged
merged 1 commit into from
Dec 7, 2023
Merged

Conversation

stephane-airbyte
Copy link
Contributor

@stephane-airbyte stephane-airbyte commented Nov 28, 2023

using enum instead of strings for postgres container modifiers

Copy link

vercel bot commented Nov 28, 2023

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 Dec 7, 2023 7:40pm

Copy link
Contributor

Before Merging a Connector Pull Request

Wow! What a great pull request you have here! 🎉

To merge this PR, ensure the following has been done/considered for each connector added or updated:

  • PR name follows PR naming conventions
  • Breaking changes are considered. If a Breaking Change is being introduced, ensure an Airbyte engineer has created a Breaking Change Plan.
  • Connector version has been incremented in the Dockerfile and metadata.yaml according to our Semantic Versioning for Connectors guidelines
  • You've updated the connector's metadata.yaml file any other relevant changes, including a breakingChanges entry for major version bumps. See metadata.yaml docs
  • Secrets in the connector's spec are annotated with airbyte_secret
  • All documentation files are up to date. (README.md, bootstrap.md, docs.md, etc...)
  • Changelog updated in docs/integrations/<source or destination>/<name>.md with an entry for the new version. See changelog example
  • Migration guide updated in docs/integrations/<source or destination>/<name>-migrations.md with an entry for the new version, if the version is a breaking change. See migration guide example
  • If set, you've ensured the icon is present in the platform-internal repo. (Docs)

If the checklist is complete, but the CI check is failing,

  1. Check for hidden checklists in your PR description

  2. Toggle the github label checklist-action-run on/off to re-run the checklist CI.

@stephane-airbyte stephane-airbyte force-pushed the 11-27-introduce_PostgresBaseImage branch from ec394c4 to b34a64a Compare November 28, 2023 21:01
@stephane-airbyte stephane-airbyte force-pushed the 11-27-introduce_ImageModifier branch from 4360a3f to e40bba3 Compare November 28, 2023 21:02
@stephane-airbyte stephane-airbyte force-pushed the 11-27-introduce_ImageModifier branch from 709a565 to 42cbb0b Compare November 28, 2023 21:53
@stephane-airbyte stephane-airbyte force-pushed the 11-27-introduce_PostgresBaseImage branch from b34a64a to 9c55439 Compare November 28, 2023 22:02
@stephane-airbyte stephane-airbyte force-pushed the 11-27-introduce_ImageModifier branch from 6c20b52 to e8694b2 Compare November 28, 2023 22:02
@stephane-airbyte stephane-airbyte force-pushed the 11-27-introduce_PostgresBaseImage branch from 9c55439 to 1d1c6d8 Compare November 28, 2023 22:19
@stephane-airbyte stephane-airbyte marked this pull request as ready for review November 28, 2023 22:19
@stephane-airbyte stephane-airbyte requested a review from a team as a November 28, 2023 22:19
@stephane-airbyte stephane-airbyte force-pushed the 11-27-introduce_ImageModifier branch from 01d903c to 0e5a21f Compare November 28, 2023 22:19
@@ -36,8 +36,27 @@ public static enum BaseImage {

}

static public PostgresTestDatabase in(BaseImage imageName, String... methods) {
final var container = new PostgresContainerFactory().shared(imageName.reference, methods);
public static enum ImageModifier {
Copy link
Contributor

Choose a reason for hiding this comment

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

This might be pedantic but this should be named ContainerModifier. We're not modifying the image.

ImageModifier(String methodName) {
this.methodName = methodName;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: add a method apply (or whatever) which actually does the thing instead of naming a method in PostgresContainerFactory. You could pass a Consumer<GenericContainer<?>> to the constructor instead. This way you get rid of an unnecessary level of indirection. It's not like the Factory object will ever be stateful anyway.


String methodName;

ImageModifier(String methodName) {
Copy link
Contributor

Choose a reason for hiding this comment

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

private?

Copy link
Contributor

@postamar postamar left a comment

Choose a reason for hiding this comment

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

The only blocker on my end is s/ImageModifier/ContainerModifier/

@stephane-airbyte stephane-airbyte force-pushed the 11-27-introduce_ImageModifier branch from 0e5a21f to 5b538b4 Compare November 29, 2023 17:58
@octavia-squidington-iv octavia-squidington-iv requested a review from a team November 29, 2023 18:00
@stephane-airbyte stephane-airbyte force-pushed the 11-27-introduce_PostgresBaseImage branch from 1d1c6d8 to d0ab8c4 Compare November 29, 2023 19:03
@stephane-airbyte stephane-airbyte force-pushed the 11-27-introduce_ImageModifier branch from 5b538b4 to ee7be9a Compare November 29, 2023 19:03
This was referenced Nov 29, 2023
@stephane-airbyte stephane-airbyte force-pushed the 11-27-introduce_PostgresBaseImage branch from e44faee to f3fb57a Compare December 2, 2023 17:44
@stephane-airbyte stephane-airbyte force-pushed the 11-27-introduce_ImageModifier branch from 4a4d2b4 to 90597c4 Compare December 2, 2023 17:44
@stephane-airbyte stephane-airbyte force-pushed the 11-27-introduce_PostgresBaseImage branch from f3fb57a to 8d01d2a Compare December 3, 2023 17:08
@stephane-airbyte stephane-airbyte force-pushed the 11-27-introduce_ImageModifier branch 2 times, most recently from 30a7d4a to 9ec7ab8 Compare December 3, 2023 17:59
@stephane-airbyte stephane-airbyte force-pushed the 11-27-introduce_PostgresBaseImage branch from 8d01d2a to e291199 Compare December 3, 2023 22:36
@stephane-airbyte stephane-airbyte force-pushed the 11-27-introduce_ImageModifier branch from 9ec7ab8 to f85604b Compare December 3, 2023 22:36
@stephane-airbyte stephane-airbyte force-pushed the 11-27-introduce_PostgresBaseImage branch from e291199 to d64b3f9 Compare December 5, 2023 19:33
@stephane-airbyte stephane-airbyte force-pushed the 11-27-introduce_ImageModifier branch from f85604b to 1408f56 Compare December 5, 2023 19:34
@octavia-squidington-iv octavia-squidington-iv requested review from a team December 5, 2023 19:35
@katmarkham katmarkham removed the request for review from a team December 6, 2023 17:38
@stephane-airbyte stephane-airbyte force-pushed the 11-27-introduce_PostgresBaseImage branch from 2981166 to d8f738c Compare December 7, 2023 01:27
@stephane-airbyte stephane-airbyte force-pushed the 11-27-introduce_ImageModifier branch from 7ca9aaf to 7496370 Compare December 7, 2023 04:08
@stephane-airbyte stephane-airbyte force-pushed the 11-27-introduce_PostgresBaseImage branch from 90780e5 to 08b4ae1 Compare December 7, 2023 04:24
@stephane-airbyte stephane-airbyte force-pushed the 11-27-introduce_ImageModifier branch from 7496370 to d108773 Compare December 7, 2023 04:24
@stephane-airbyte stephane-airbyte force-pushed the 11-27-introduce_PostgresBaseImage branch from 08b4ae1 to a0b2b49 Compare December 7, 2023 04:49
@stephane-airbyte stephane-airbyte force-pushed the 11-27-introduce_ImageModifier branch from d108773 to 7aa296c Compare December 7, 2023 04:50
@stephane-airbyte stephane-airbyte force-pushed the 11-27-introduce_PostgresBaseImage branch from a0b2b49 to f8673f9 Compare December 7, 2023 06:32
@stephane-airbyte stephane-airbyte force-pushed the 11-27-introduce_ImageModifier branch from 7aa296c to 1995d70 Compare December 7, 2023 06:32
@stephane-airbyte stephane-airbyte force-pushed the 11-27-introduce_PostgresBaseImage branch from d1cbe9f to 42021c4 Compare December 7, 2023 16:41
@stephane-airbyte stephane-airbyte force-pushed the 11-27-introduce_ImageModifier branch from b93ad8e to b89abae Compare December 7, 2023 16:41
Copy link
Contributor Author

stephane-airbyte commented Dec 7, 2023

Merge activity

Base automatically changed from 11-27-introduce_PostgresBaseImage to master December 7, 2023 19:40
@stephane-airbyte stephane-airbyte force-pushed the 11-27-introduce_ImageModifier branch from b89abae to 460d08d Compare December 7, 2023 19:40
Copy link
Contributor

github-actions bot commented Dec 7, 2023

Coverage report for source-postgres

There is no coverage information present for the Files changed

Total Project Coverage 71.66% 🍏

@stephane-airbyte stephane-airbyte merged commit f1b91b3 into master Dec 7, 2023
23 of 35 checks passed
@stephane-airbyte stephane-airbyte deleted the 11-27-introduce_ImageModifier branch December 7, 2023 19:53
rishabh-cldcvr pushed a commit to ollionorg/datapipes-airbyte that referenced this pull request Dec 14, 2023
using enum instead of strings for postgres container modifiers
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants