-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
connectors-ci: deprecate slash publish #25865
connectors-ci: deprecate slash publish #25865
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few small things! Otherwise looks good
@@ -31,7 +31,7 @@ Our SSH connector support is designed to be easy to plug into any existing conne | |||
Replace port_key and host_key as necessary. Look at `transform_postgres()` to see an example. | |||
2. To make sure your changes are present in Normalization when running tests on the connector locally, you'll need to change [this version tag](https://github.com/airbytehq/airbyte/blob/6d9ba022646441c7f298ca4dcaa3df59b9a19fbb/airbyte-workers/src/main/java/io/airbyte/workers/normalization/DefaultNormalizationRunner.java#L50) to `dev` so that the new locally built docker image for Normalization is used. Don't push this change with the PR though. | |||
3. If your `host_key="host"` and `port_key="port"` then this step is not necessary. However if the key names differ for your connector, you will also need to add some logic into `sshtunneling.sh` (within airbyte-workers) to handle this, as currently it assumes that the keys are exactly `host` and `port`. | |||
4. When making your PR, make sure that you've version bumped Normalization (in `airbyte-workers/src/main/java/io/airbyte/workers/normalization/DefaultNormalizationRunner.java` and `airbyte-integrations/bases/base-normalization/Dockerfile`). You'll need to /test & /publish Normalization _first_ so that when you /test the connector, it can use the new version. | |||
4. When making your PR, make sure that you've version bumped Normalization (in `airbyte-workers/src/main/java/io/airbyte/workers/normalization/DefaultNormalizationRunner.java` and `airbyte-integrations/bases/base-normalization/Dockerfile`). You'll need to /test & /legacy-publish Normalization _first_ so that when you /test the connector, it can use the new version. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alafanechere why do we still tell them to use legacy?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Normalization publish is not yet implemented in our CI pipelines ...
* `/publish connector=connectors/source-sendgrid` - Publish the docker image if it doesn't exist for a single connector on the latest PR commit. | ||
|
||
### 5. Automatically Run From `master` | ||
### 4. Automatically Run From `master` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two things.
- Should we not still mention
/test
- Should me mention in the merge to master section that the connector will be published?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a "PR blurb" going out to devs to tell them what the new workflow is? would love to see that too
@@ -27,6 +27,7 @@ jobs: | |||
build-connector | |||
publish-connector | |||
publish |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so is the /publish
command still available after this PR? which workflow file does it use/how does it know to use it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This worfklow is responsible for writing the deprecation message: .github/workflows/publish-command.yml
Commands are mapped to workflow via the -command
suffix in their filename.
|
||
Connector icons should be square SVGs and be located in [this directory](https://github.com/airbytehq/airbyte/tree/master/airbyte-config-oss/init-oss/src/main/resources/icons). | ||
|
||
Connector documentation and changelogs are markdown files which live either [here for sources](https://github.com/airbytehq/airbyte/tree/master/docs/integrations/sources), or [here for destinations](https://github.com/airbytehq/airbyte/tree/master/docs/integrations/destinations). | ||
|
||
### The /publish command |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since normalization people are still using this, could we keep this reference around somewhere? maybe just in the normalization README?
|
This reverts commit b008f44.
What
Closes #25432
As we move forward with "publish connectors on merge" we should deprecate the /publish command.
How
publish-connectors.yml
worfklow release non pre-prerelease imagespublish-command.yml
tolegacy-publish-command.yml
so that /publish does not run the usual worfklow./legacy-publish
can be used using the old/publish
is needed (e.g: normalization publication)publish-command.yml
post a deprecationg message so that /publish use leads to deprecation message comment./publishj
User impact