Remove redundant title labels from connector specs #17544

evantahler · 2022-10-03T23:30:38Z

Closes #17182
Now that our UI has been updated to label optional fields, we no longer need to say "field (optional)" in our connector specs. This PR makes the changes needed, and does not require the re-publishing of the effected connectors.

Note for reviewers:

Here's the theory - If I manually change both the connector spec files and source_specs.yaml and destination_specs.yaml manually, we don't need to republish all the connectors. The application reads the yaml files for the UI, which was the main concern here.
As part of this PR, I added the new control Environment Variable, GITHUB_STORE_BRANCH which can be used to point the AirbyteGithubStore class at another branch - this means that it's possible to test the changes above locally with:

# push the changes to your branch
docker system prune -a && docker volume prune # start empty
SUB_BUILD=PLATFORM ./gradlew build -x test # build the platorm
GITHUB_STORE_BRANCH="evan/connector-labels" VERSION=dev docker-compose up

(From destination-bigquery)

ryankfu

removing redundancy and adding the ability to test locally! 😃

Not blocking, however I'm curious since you've mentioned that a linter was used to clean up is that a rule that can be codified to prevent the usage of (Optional) getting added int the spec files?

Additionally, should there be a place where the sweet changes to GITHUB_STORE_BRANCH be added like some documentation maybe? I can easily see this feature getting easily lost over time unless people are inspecting EnvConfigs.java

evantahler · 2022-10-05T00:21:01Z

I removed the note about the linter - I was having conflicts between how our build tool and VSCode wanted to format the YML file.

I don't think we need a linter per-se for these (optional) tags - they are all historic, and were needed to add clarity in the UI until last week when @lmossman's UI updates were merged in.

I too agree that we have a LOT of Environment variables one could use to customize things... maybe @jdpgrailsdev has an idea of how to organize these? I know Micronaut has a much better config system - perhaps we wait until the server get's moved over to Micronaut.

alafanechere

LGTM but this raised two questions from my side:

In which situation do we want cut a new connector version when a connector spec is updated
@pedroslopez how is the GitHubStore is conceptually different from the remote catalog we'll have on the cloud apart from the masking abilities of our remote catalog?

alafanechere · 2022-10-05T10:15:40Z

airbyte-server/src/main/java/io/airbyte/server/services/AirbyteGithubStore.java

  private static final String GITHUB_BASE_URL = "https://raw.githubusercontent.com";
  private static final String SOURCE_DEFINITION_LIST_LOCATION_PATH =
-      "/airbytehq/airbyte/master/airbyte-config/init/src/main/resources/seed/source_definitions.yaml";
+      "/airbytehq/airbyte/" + envConfigs.getGithubStoreBranch() + "/airbyte-config/init/src/main/resources/seed/source_definitions.yaml";


What do you think about expanding the env var?
You could make the default value be:
/airbytehq/airbyte/master/airbyte-config/init/src/main/resources/seed/

It would allow OSS users to set a custom github store on their own repo.

I like it, but that seems like a specific feature we should think more about (are there security implications? Do we want that in the UI?)... so it should be in another PR at least :D

jdpgrailsdev · 2022-10-05T19:04:37Z

I removed the note about the linter - I was having conflicts between how our build tool and VSCode wanted to format the YML file.

I don't think we need a linter per-se for these (optional) tags - they are all historic, and were needed to add clarity in the UI until last week when @lmossman's UI updates were merged in.

I too agree that we have a LOT of Environment variables one could use to customize things... maybe @jdpgrailsdev has an idea of how to organize these? I know Micronaut has a much better config system - perhaps we wait until the server get's moved over to Micronaut.

@evantahler I agree that we have too many environment variables. One solution once we move to Micronaut is to leverage Environments. They are basically labels that can be used to conditionally load configuration and make choices at runtime. I've noticed that a lot of our env vars are basically just these, with the extra layer of having to know the name of the env var to look at (for example: DEPLOYMENT_MODE, WORKER_ENVIRONMENT, AIRBYTE_ROLE, etc). All of these could and can be reduced to an environment: MICRONAUT_ENVIRONMENTS=production,oss,docker,... In addition, as you point out, the YAML configuration file with declared defaults is much easier to read/find out what env var is used than the existing configs class.

gosusnp · 2022-10-05T22:57:44Z

docker-compose.yaml

@@ -138,6 +138,7 @@ services:
      - WEBAPP_URL=${WEBAPP_URL}
      - WORKER_ENVIRONMENT=${WORKER_ENVIRONMENT}
      - WORKSPACE_ROOT=${WORKSPACE_ROOT}
+      - GITHUB_STORE_BRANCH=${GITHUB_STORE_BRANCH}


Randomly landed here: if this is needed for cloud, we should probably update kustomize and helm charts as well.

It's not needed for ☁️!

…vation * master: (26 commits) supply a source id for schema discovery in connector integration tests (#17662) Source Iterable: Add permission check for stream (#17602) Moving TrackingClientSingleton.initialize into the bean itself (#17631) Handle null workspace IDs in tracking/reporting methods gracefully (#17641) Bump Airbyte version from 0.40.11 to 0.40.12 (#17653) Revert "Do not wait the end of a reset to return an update (#17591)" (#17640) Standardize HttpRequester's url_base and path format (#17524) Create geography_type enum and add geography column in connection and workspace table (#16818) airbyte-cron: update connector definitions from remote (#16438) Do not wait the end of a reset to return an update (#17591) Remove redundant title labels from connector specs (#17544) Updated GA4 status support large schema discovery (#17394) 🪟 🐛 Fixes connector checks not properly ending their loading state (#17620) 🪟🧪 [Experiment] add hideOnboarding experiment (#17605) Source Recharge: change releaseStage to GA (#17606) Source Recharge: skip stream if 403 received (#17608) remove sonar-scan workflow (#17609) Mark/tables should be full width on all pages (#17401) Auto fail all workfow if there is a Versioning issue (#17562) ...

* Remove redundant title labels from connector specs * Manually update specs * add env variable * Remove debugging log

Remove redundant title labels from connector specs

7b3c524

github-actions bot added the area/connectors Connector related issues label Oct 3, 2022

add env variable

410fa7c

github-actions bot added area/platform issues related to the platform area/server labels Oct 4, 2022

Remove debugging log

8611290

evantahler requested review from tuliren, alafanechere, pedroslopez and lmossman October 4, 2022 23:56

evantahler marked this pull request as ready for review October 4, 2022 23:56

evantahler requested a review from a team as a code owner October 4, 2022 23:56

airbytehq deleted a comment from github-actions bot Oct 4, 2022

evantahler temporarily deployed to more-secrets October 4, 2022 23:57 Inactive

ryankfu approved these changes Oct 5, 2022

View reviewed changes

alafanechere approved these changes Oct 5, 2022

View reviewed changes

alafanechere reviewed Oct 5, 2022

View reviewed changes

evantahler merged commit 49cb336 into master Oct 5, 2022

evantahler deleted the evan/connector-labels branch October 5, 2022 19:58

octavia-squidington-iii mentioned this pull request Oct 5, 2022

Bump Airbyte version from 0.40.11 to 0.40.12 #17635

Closed

gosusnp reviewed Oct 5, 2022

View reviewed changes

octavia-squidington-iii mentioned this pull request Oct 6, 2022

Bump Airbyte version from 0.40.11 to 0.40.12 #17653

Merged

subodh1810 mentioned this pull request Oct 10, 2022

postgres-source: handle 24:00:00 value for time column #17782

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove redundant title labels from connector specs #17544

Remove redundant title labels from connector specs #17544

evantahler commented Oct 3, 2022 •

edited

Loading

ryankfu left a comment

evantahler commented Oct 5, 2022

alafanechere left a comment •

edited

Loading

alafanechere Oct 5, 2022

evantahler Oct 5, 2022

jdpgrailsdev commented Oct 5, 2022

gosusnp Oct 5, 2022

evantahler Oct 5, 2022

Remove redundant title labels from connector specs #17544

Remove redundant title labels from connector specs #17544

Conversation

evantahler commented Oct 3, 2022 • edited Loading

ryankfu left a comment

Choose a reason for hiding this comment

evantahler commented Oct 5, 2022

alafanechere left a comment • edited Loading

Choose a reason for hiding this comment

alafanechere Oct 5, 2022

Choose a reason for hiding this comment

evantahler Oct 5, 2022

Choose a reason for hiding this comment

jdpgrailsdev commented Oct 5, 2022

gosusnp Oct 5, 2022

Choose a reason for hiding this comment

evantahler Oct 5, 2022

Choose a reason for hiding this comment

evantahler commented Oct 3, 2022 •

edited

Loading

alafanechere left a comment •

edited

Loading