airbyte-ci: add format commands #31831

erohmensing · 2023-10-25T17:57:11Z

What

Move formatting to the top of the repository and achieved via airbyte-ci
Remove formatting in connector testing - no more sketchy auto-commits
Make formatting faster!
close airbyte-ci: top level formatting replaces connector-level formatting and gradle format ci job #32061

How

New airbyte-ci format fix and airbyte-ci format check subgroups
New commands which run (daggerized):
- Java + sql: spotless, via gradle, in a JDK container
- Yaml/Json: prettier, not via gradle, in a node container
- License checking: License checking via a new tool, addlicense, in a go container (also no longer through gradle)
- Python: black and isort, in a python container, no longer via gradle
Remove everything possible that doesn't touch java code out of gradle
Remove formatting from connector testing
Remove format command from gradle and the gradle check workflow - format checking and formatting should only happen via ci (gradle does other checking still)

Usage

General workflow:
0. Dev pushes a PR

If they didn't format something correctly, the workflow fails
The user sees that they should run the formatter
They run the formatter and commit it
Success

Other ways we can increase the ease of formatting:

Instructions on how to set up various IDEs (intellij, vscode) with the new top-level pyproject.toml files to autoformat
Pre-commit or pre-push hooks (i'm partial to the latter) that devs can opt into

Todo

This new check should be required, even before octavia approvington. It takes ~3 mins to run. Making sure that each PR is formatted is the key to keeping master from getting into a weird state where formatting seems to "ghost format" code that you didn't touch. E.g. this should feel similar to the current workflow in platform

🚨 User Impact 🚨

New functionality in airbyte-ci :)
No more autoformatting commits on PRs
Users may see formatting fail in CI and need to fix it locally (similar to in the platform repo)

…line-context

vercel · 2023-10-25T17:57:18Z

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		Nov 13, 2023 9:14am

erohmensing · 2023-10-25T17:57:22Z

Current dependencies on/for this PR:

master
- PR airbyte-ci: add format commands #31831 👈
  - PR format: introduce CommandResult, reuse step status, make concurrent command invokation more generic #32296

This stack of pull requests is managed by Graphite.

alafanechere · 2023-10-26T18:51:36Z

airbyte-ci/connectors/pipelines/pipelines/airbyte_ci/format/java/commands.py

+async def format_java(fix: bool) -> bool:
+    logger = logging.getLogger("format")
+    if fix:
+        gradle_command = ["./gradlew", "spotlessApply"]
+    else:
+        gradle_command = ["./gradlew", "spotlessCheck", "--scan"]
+
+    async with dagger.Connection(dagger.Config(log_output=sys.stderr)) as dagger_client:
+        try:
+            format_container = await (
+                dagger_client.container()
+                .from_("openjdk:17.0.1-jdk-slim")
+                .with_exec(
+                    sh_dash_c(
+                        [
+                            "apt-get update",
+                            "apt-get install -y bash",
+                        ]
+                    )
+                )
+                .with_mounted_directory(
+                    "/src",
+                    dagger_client.host().directory(
+                        ".",
+                        include=[
+                            "**/*.java",
+                            "**/*.sql",
+                            "**/*.gradle",
+                            "gradlew",
+                            "gradlew.bat",
+                            "gradle",
+                            "**/deps.toml",
+                            "**/gradle.properties",
+                            "**/version.properties",
+                            "tools/gradle/codestyle/java-google-style.xml",
+                            "tools/gradle/codestyle/sql-dbeaver.properties",
+                        ],
+                        exclude=DEFAULT_FORMAT_IGNORE_LIST,
+                    ),
+                )
+                .with_workdir(f"/src")
+                .with_exec(gradle_command)
+            )
+
+            await format_container
+            if fix:
+                await format_container.directory("/src").export(".")
+
+            return True
+        except dagger.ExecError as e:
+            logger.error("Format failed")
+            logger.error(e.stderr)
+            return False


👋 This is what I meant in your demo when I asked "are you using the Python GradleTask class" (I know it's utterly confusing when said orally 🤣 ).
Re-using this class (or just its methods) would make sure we have a standard way of running the gradle commands in dagger. We recently improved it to make sure subsequent runs benefit from caching.

I realize that with Lazy subcommand we're stepping away from the Step and pipeline paradigm which happens in the connector subgroup.
I find this new approach elegant, nevertheless, I'd appreciate code reuse.
Feel free to split this class up to not make all its internal logic depend on the Step one and pick the things you need from it.

Oh this one! true! Great point, I'll take a look :)

I gave this a try - it was really hard without knowing what's really going on in here. The current code although its a "gradle task" command is highly configured to handle gradle commands in connector containers.

I personally would feel uncomfortable messing around with this too much myself. If you want to stack off of this to split up the gradleTask into a gradle base container in which i can run gradle commands (not just connector gradle commands), that'd be very welcome!

Here's the commit where I took a crack; it was also hard with the PipelineContext etc,:
a22a14b

This is a sign that some further refactoring is required for this change to land properly. I'm not surprised that the PipelineContext and friends get in the way, and that they would need to be changed at some point. Perhaps the time has come, though that would be outside the scope of this PR.

alafanechere · 2023-10-26T18:54:45Z

airbyte-ci/connectors/pipelines/pipelines/airbyte_ci/format/js/commands.py

+                .from_("node:18.18.0-slim")
+                .with_exec(
+                    sh_dash_c(
+                        [
+                            "apt-get update",
+                            "apt-get install -y bash",
+                        ]
+                    )
+                )


Can this be declared a dagger/action/?module and we'd call: with_node(dagger_client, node_version)?

Dumb question - that would be a container, not an action, right?

I was tripped up by most containers in dagger actions taking in pipelineContexts, not dagger_clients.

or is it dagger.container().with_go? 🤔

Without the apt-get updating i've removed, that method would just be

return container.from_(NODE_IMAGE) I think

alafanechere · 2023-10-26T18:55:28Z

airbyte-ci/connectors/pipelines/pipelines/airbyte_ci/format/license/commands.py

+        try:
+            license_container = await (
+                dagger_client.container()
+                .from_("golang:1.17")


Same comment as for node: a with_go(dagger_client, go_version) action function call would be nice

airbyte-ci/connectors/pipelines/pipelines/airbyte_ci/format/python/commands.py

airbyte-ci/connectors/pipelines/pipelines/airbyte_ci/format/commands.py

…line-context

postamar

I did another pass. LGTM

This reverts commit 969d23f.

alafanechere

🎉

github-actions · 2023-11-14T00:43:24Z

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,

Check for hidden checklists in your PR description
Toggle the github label checklist-action-run on/off to re-run the checklist CI.

alafanechere · 2023-11-14T08:17:04Z

/approve-and-merge reason="bypass connectors tests - formatting changes only"

octavia-approvington · 2023-11-14T08:17:46Z

Enough! Let's do this

bnchrch and others added 13 commits October 19, 2023 14:24

Add lazy decorator

a4994f4

Add playground subcommands

ccddd26

steal

96643f1

Show demo of context coming through

efa0786

Add pass_click_context_and_args_to_children

76458dc

Add click decorator to reduce boilerplate

58845d3

Add click_append_to_context_object decorator

435f44d

Move files

55baf8f

Automated Commit - Formatting Changes

8bfe2aa

Merge remote-tracking branch 'origin/master' into bnchrch/simple-pipe…

a9b7365

…line-context

Bring up to date with async click

27043ae

Replace test with playground impl

c53310a

Add singleton test

b07da00

octavia-squidington-iii added area/octavia-cli normalization labels Oct 25, 2023

alafanechere reviewed Oct 26, 2023

View reviewed changes

erohmensing force-pushed the ella/format-commands branch from 40c3295 to 6235d20 Compare October 30, 2023 20:08

bnchrch and others added 3 commits October 30, 2023 16:56

Merge remote-tracking branch 'origin/master' into bnchrch/simple-pipe…

03a14c9

…line-context

Fix low hanging comments

714d201

Automated Commit - Formatting Changes

29087ea

erohmensing force-pushed the ella/format-commands branch from 6235d20 to d685a6c Compare October 31, 2023 17:50

erohmensing changed the base branch from master to bnchrch/simple-pipeline-context October 31, 2023 17:50

bnchrch added 5 commits October 31, 2023 16:49

Move decorator to file

fb43cfb

Add tests

323139d

Add forgotten test

f624fa3

Update ctx to click pipeline context

24297df

Add cli tests

58b28af

Merge branch 'master' into ella/format-commands

bda56e5

vercel bot deployed to Preview November 10, 2023 10:14 View deployment

format additional files

a6a1d37

octavia-squidington-iii added the connectors/source/intercom label Nov 10, 2023

alafanechere and others added 4 commits November 10, 2023 12:14

remove format from gradle.yml

76f83ea

gradle: tweak spotless config

0bcf0fc

remove octavia-cli from gradle build

c48e9c9

gradle: restore commented block in base-normalization

749ffab

octavia-squidington-iii removed the normalization label Nov 10, 2023

postamar approved these changes Nov 10, 2023

View reviewed changes

fix base-normalization issue

139f809

octavia-squidington-iii added the normalization label Nov 10, 2023

bnchrch and others added 2 commits November 10, 2023 13:01

Add auto format

84ff3b5

Merge branch 'master' into ella/format-commands

02ca702

vercel bot deployed to Preview November 13, 2023 08:44 View deployment

alafanechere added 2 commits November 13, 2023 09:59

bump git-auto-commit-action to v5

89f1b2c

to revert: test format

8385bcf

octavia-squidington-iii added the connectors/source/pokeapi label Nov 13, 2023

alafanechere and others added 3 commits November 13, 2023 09:08

Automated Commit - Formatting Changes

969d23f

Revert "Automated Commit - Formatting Changes"

84fd85a

This reverts commit 969d23f.

Automated Commit - Formatting Changes

9c34b9d

alafanechere approved these changes Nov 13, 2023

View reviewed changes

stephane-airbyte closed this Nov 14, 2023

stephane-airbyte reopened this Nov 14, 2023

octavia-approvington approved these changes Nov 14, 2023

View reviewed changes

octavia-approvington merged commit ac3eb28 into master Nov 14, 2023
34 of 37 checks passed

octavia-approvington deleted the ella/format-commands branch November 14, 2023 08:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

airbyte-ci: add format commands #31831

airbyte-ci: add format commands #31831

erohmensing commented Oct 25, 2023 •

edited

Loading

vercel bot commented Oct 25, 2023 •

edited

Loading

erohmensing commented Oct 25, 2023 •

edited by alafanechere

Loading

alafanechere Oct 26, 2023 •

edited

Loading

erohmensing Oct 26, 2023

erohmensing Nov 6, 2023

postamar Nov 6, 2023

alafanechere Oct 26, 2023

erohmensing Nov 6, 2023

alafanechere Oct 26, 2023

postamar left a comment

alafanechere left a comment

github-actions bot commented Nov 14, 2023

alafanechere commented Nov 14, 2023

octavia-approvington commented Nov 14, 2023

airbyte-ci: add format commands #31831

airbyte-ci: add format commands #31831

Conversation

erohmensing commented Oct 25, 2023 • edited Loading

What

How

Usage

Todo

Recommended reading order

🚨 User Impact 🚨

vercel bot commented Oct 25, 2023 • edited Loading

erohmensing commented Oct 25, 2023 • edited by alafanechere Loading

alafanechere Oct 26, 2023 • edited Loading

Choose a reason for hiding this comment

erohmensing Oct 26, 2023

Choose a reason for hiding this comment

erohmensing Nov 6, 2023

Choose a reason for hiding this comment

postamar Nov 6, 2023

Choose a reason for hiding this comment

alafanechere Oct 26, 2023

Choose a reason for hiding this comment

erohmensing Nov 6, 2023

Choose a reason for hiding this comment

alafanechere Oct 26, 2023

Choose a reason for hiding this comment

postamar left a comment

Choose a reason for hiding this comment

alafanechere left a comment

Choose a reason for hiding this comment

github-actions bot commented Nov 14, 2023

Before Merging a Connector Pull Request

alafanechere commented Nov 14, 2023

octavia-approvington commented Nov 14, 2023

erohmensing commented Oct 25, 2023 •

edited

Loading

vercel bot commented Oct 25, 2023 •

edited

Loading

erohmensing commented Oct 25, 2023 •

edited by alafanechere

Loading

alafanechere Oct 26, 2023 •

edited

Loading