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

fix: proplerly render extraContainers #16474

Merged
merged 10 commits into from
Sep 13, 2022
Merged

fix: proplerly render extraContainers #16474

merged 10 commits into from
Sep 13, 2022

Conversation

bdashrad
Copy link
Contributor

@bdashrad bdashrad commented Sep 8, 2022

Signed-off-by: Brad Clark bdashrad@gmail.com

What

Fixes #16470

How

Apply proper indentation when rendering extraContainers

Recommended reading order

🚨 User Impact 🚨

Bug fix, no breaking changes

Pre-merge Checklist

Community member or Airbyter

Airbyter

If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.

  • Create a non-forked branch based on this PR and test the below items on it
  • Build is successful
  • If new credentials are required for use in CI, add them to GSM. Instructions.
  • /test connector=connectors/<name> command is passing
  • New Connector version released on Dockerhub by running the /publish command described here
  • After the connector is published, connector added to connector index as described here
  • Seed specs have been re-generated by building the platform and committing the changes to the seed spec files, as described here
Updating a connector

Community member or Airbyter

  • Grant edit access to maintainers (instructions)
  • Secrets in the connector's spec are annotated with airbyte_secret
  • Unit & integration tests added and passing. Community members, please provide proof of success locally e.g: screenshot or copy-paste unit, integration, and acceptance test output. To run acceptance tests for a Python connector, follow instructions in the README. For java connectors run ./gradlew :airbyte-integrations:connectors:<name>:integrationTest.
  • Code reviews completed
  • Documentation updated
    • Connector's README.md
    • Connector's bootstrap.md. See description and examples
    • Changelog updated in docs/integrations/<source or destination>/<name>.md including changelog. See changelog example
  • PR name follows PR naming conventions

Airbyter

If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.

  • Create a non-forked branch based on this PR and test the below items on it
  • Build is successful
  • If new credentials are required for use in CI, add them to GSM. Instructions.
  • /test connector=connectors/<name> command is passing
  • New Connector version released on Dockerhub and connector version bumped by running the /publish command described here
Connector Generator
  • Issue acceptance criteria met
  • PR name follows PR naming conventions
  • If adding a new generator, add it to the list of scaffold modules being tested
  • The generator test modules (all connectors with -scaffold in their name) have been updated with the latest scaffold by running ./gradlew :airbyte-integrations:connector-templates:generator:testScaffoldTemplates then checking in your changes
  • Documentation which references the generator is updated as needed

Tests

Unit

Put your unit tests output here.

Integration

Put your integration tests output here.

Acceptance

Put your acceptance tests output here.

Signed-off-by: Brad Clark <bdashrad@gmail.com>
@CLAassistant
Copy link

CLAassistant commented Sep 8, 2022

CLA assistant check
All committers have signed the CLA.

@github-actions github-actions bot added area/platform issues related to the platform kubernetes labels Sep 8, 2022
@bdashrad
Copy link
Contributor Author

bdashrad commented Sep 8, 2022

It's unclear to me where the charts are actually stored, because changes in this code repo don't line up with the helm chart repo.

I'm also happy to run the helm-docs commands to update the README if they are run in ci/cd.

@bdashrad
Copy link
Contributor Author

bdashrad commented Sep 8, 2022

I have run chart-testing locally, and there are some additional changes it would like to see, around newlines in values.yaml, version pinning in https://github.com/airbytehq/airbyte/blob/master/charts/airbyte/Chart.yaml#L26.

The airbyte-metrics chart is also missing from the helm chart repo, which causes further errors.

chart-testing
$ ct lint charts/airbyte
Linting charts...

------------------------------------------------------------------------------------------------------------------------
 Charts to be processed:
------------------------------------------------------------------------------------------------------------------------
 metrics => (version: "0.45.2", path: "charts/airbyte-metrics")
 server => (version: "0.45.2", path: "charts/airbyte-server")
 temporal => (version: "0.45.2", path: "charts/airbyte-temporal")
 webapp => (version: "0.45.2", path: "charts/airbyte-webapp")
 worker => (version: "0.45.2", path: "charts/airbyte-worker")
 airbyte => (version: "0.45.2", path: "charts/airbyte")
------------------------------------------------------------------------------------------------------------------------

Hang tight while we grab the latest from your chart repositories...
...Successfully got an update from the "runatlantis" chart repository
...Successfully got an update from the "ingress-nginx" chart repository
...Successfully got an update from the "privatebin" chart repository
...Successfully got an update from the "falcosecurity" chart repository
...Successfully got an update from the "jetstack" chart repository
...Successfully got an update from the "doppler" chart repository
...Successfully got an update from the "bdashrad" chart repository
...Successfully got an update from the "skm" chart repository
...Successfully got an update from the "argo" chart repository
...Successfully got an update from the "datadog" chart repository
...Successfully got an update from the "airbyte-oss" chart repository
...Successfully got an update from the "incubator" chart repository
...Successfully got an update from the "bitnami" chart repository
...Successfully got an update from the "redis" chart repository
...Successfully got an update from the "stable" chart repository
Update Complete. ⎈Happy Helming!⎈
Saving 1 charts
Downloading common from repo https://charts.bitnami.com/bitnami
Deleting outdated charts
Linting chart 'metrics => (version: "0.45.2", path: "charts/airbyte-metrics")'
Checking chart 'metrics => (version: "0.45.2", path: "charts/airbyte-metrics")' for a version bump...
Old chart version: 0.39.36
New chart version: 0.45.2
Chart version ok.
Validating /Users/bradclark/code/airbyte/charts/airbyte-metrics/Chart.yaml...
Validation success! 👍
charts/airbyte-metrics/values.yaml
  1:1       error    too many blank lines (1 > 0)  (empty-lines)

Hang tight while we grab the latest from your chart repositories...
...Successfully got an update from the "bdashrad" chart repository
...Successfully got an update from the "privatebin" chart repository
...Successfully got an update from the "doppler" chart repository
...Successfully got an update from the "skm" chart repository
...Successfully got an update from the "falcosecurity" chart repository
...Successfully got an update from the "jetstack" chart repository
...Successfully got an update from the "runatlantis" chart repository
...Successfully got an update from the "ingress-nginx" chart repository
...Successfully got an update from the "argo" chart repository
...Successfully got an update from the "incubator" chart repository
...Successfully got an update from the "airbyte-oss" chart repository
...Successfully got an update from the "datadog" chart repository
...Successfully got an update from the "stable" chart repository
...Successfully got an update from the "bitnami" chart repository
...Successfully got an update from the "redis" chart repository
Update Complete. ⎈Happy Helming!⎈
Saving 1 charts
Downloading common from repo https://charts.bitnami.com/bitnami
Deleting outdated charts
Linting chart 'server => (version: "0.45.2", path: "charts/airbyte-server")'
Checking chart 'server => (version: "0.45.2", path: "charts/airbyte-server")' for a version bump...
Old chart version: 0.39.36
New chart version: 0.45.2
Chart version ok.
Validating /Users/bradclark/code/airbyte/charts/airbyte-server/Chart.yaml...
Validation success! 👍
charts/airbyte-server/values.yaml
  1:1       error    too many blank lines (1 > 0)  (empty-lines)
  193:1     error    too many blank lines (1 > 0)  (empty-lines)

Hang tight while we grab the latest from your chart repositories...
...Successfully got an update from the "doppler" chart repository
...Successfully got an update from the "privatebin" chart repository
...Successfully got an update from the "runatlantis" chart repository
...Successfully got an update from the "skm" chart repository
...Successfully got an update from the "falcosecurity" chart repository
...Successfully got an update from the "bdashrad" chart repository
...Successfully got an update from the "jetstack" chart repository
...Successfully got an update from the "ingress-nginx" chart repository
...Successfully got an update from the "argo" chart repository
...Successfully got an update from the "airbyte-oss" chart repository
...Successfully got an update from the "datadog" chart repository
...Successfully got an update from the "incubator" chart repository
...Successfully got an update from the "bitnami" chart repository
...Successfully got an update from the "stable" chart repository
...Successfully got an update from the "redis" chart repository
Update Complete. ⎈Happy Helming!⎈
Saving 1 charts
Downloading common from repo https://charts.bitnami.com/bitnami
Deleting outdated charts
Linting chart 'temporal => (version: "0.45.2", path: "charts/airbyte-temporal")'
Checking chart 'temporal => (version: "0.45.2", path: "charts/airbyte-temporal")' for a version bump...
Old chart version: 0.39.36
New chart version: 0.45.2
Chart version ok.
Validating /Users/bradclark/code/airbyte/charts/airbyte-temporal/Chart.yaml...
Validation success! 👍
charts/airbyte-temporal/values.yaml
  1:1       error    too many blank lines (1 > 0)  (empty-lines)
  155:20    error    no new line character at the end of file  (new-line-at-end-of-file)

Hang tight while we grab the latest from your chart repositories...
...Successfully got an update from the "bdashrad" chart repository
...Successfully got an update from the "runatlantis" chart repository
...Successfully got an update from the "ingress-nginx" chart repository
...Successfully got an update from the "privatebin" chart repository
...Successfully got an update from the "skm" chart repository
...Successfully got an update from the "doppler" chart repository
...Successfully got an update from the "falcosecurity" chart repository
...Successfully got an update from the "jetstack" chart repository
...Successfully got an update from the "incubator" chart repository
...Successfully got an update from the "datadog" chart repository
...Successfully got an update from the "argo" chart repository
...Successfully got an update from the "airbyte-oss" chart repository
...Successfully got an update from the "redis" chart repository
...Successfully got an update from the "stable" chart repository
...Successfully got an update from the "bitnami" chart repository
Update Complete. ⎈Happy Helming!⎈
Saving 1 charts
Downloading common from repo https://charts.bitnami.com/bitnami
Deleting outdated charts
Linting chart 'webapp => (version: "0.45.2", path: "charts/airbyte-webapp")'
Checking chart 'webapp => (version: "0.45.2", path: "charts/airbyte-webapp")' for a version bump...
Old chart version: 0.39.36
New chart version: 0.45.2
Chart version ok.
Validating /Users/bradclark/code/airbyte/charts/airbyte-webapp/Chart.yaml...
Validation success! 👍
charts/airbyte-webapp/values.yaml
  177:1     error    too many blank lines (1 > 0)  (empty-lines)

Hang tight while we grab the latest from your chart repositories...
...Successfully got an update from the "doppler" chart repository
...Successfully got an update from the "bdashrad" chart repository
...Successfully got an update from the "privatebin" chart repository
...Successfully got an update from the "runatlantis" chart repository
...Successfully got an update from the "jetstack" chart repository
...Successfully got an update from the "falcosecurity" chart repository
...Successfully got an update from the "skm" chart repository
...Successfully got an update from the "ingress-nginx" chart repository
...Successfully got an update from the "datadog" chart repository
...Successfully got an update from the "airbyte-oss" chart repository
...Successfully got an update from the "argo" chart repository
...Successfully got an update from the "incubator" chart repository
...Successfully got an update from the "bitnami" chart repository
...Successfully got an update from the "redis" chart repository
...Successfully got an update from the "stable" chart repository
Update Complete. ⎈Happy Helming!⎈
Saving 1 charts
Downloading common from repo https://charts.bitnami.com/bitnami
Deleting outdated charts
Linting chart 'worker => (version: "0.45.2", path: "charts/airbyte-worker")'
Checking chart 'worker => (version: "0.45.2", path: "charts/airbyte-worker")' for a version bump...
Old chart version: 0.39.36
New chart version: 0.45.2
Chart version ok.
Validating /Users/bradclark/code/airbyte/charts/airbyte-worker/Chart.yaml...
Validation success! 👍
Validating maintainers...
Hang tight while we grab the latest from your chart repositories...
...Successfully got an update from the "doppler" chart repository
...Successfully got an update from the "runatlantis" chart repository
...Successfully got an update from the "bdashrad" chart repository
...Successfully got an update from the "ingress-nginx" chart repository
...Successfully got an update from the "privatebin" chart repository
...Successfully got an update from the "falcosecurity" chart repository
...Successfully got an update from the "skm" chart repository
...Successfully got an update from the "datadog" chart repository
...Successfully got an update from the "argo" chart repository
...Successfully got an update from the "jetstack" chart repository
...Successfully got an update from the "airbyte-oss" chart repository
...Successfully got an update from the "incubator" chart repository
...Successfully got an update from the "bitnami" chart repository
...Successfully got an update from the "redis" chart repository
...Successfully got an update from the "stable" chart repository
Update Complete. ⎈Happy Helming!⎈
Error: dependency "airbyte-bootloader" has an invalid version/constraint format: improper constraint: placeholder
------------------------------------------------------------------------------------------------------------------------
No chart changes detected.
------------------------------------------------------------------------------------------------------------------------
Error: Error linting charts: Error building dependencies for chart 'airbyte => (version: "0.45.2", path: "charts/airbyte")': Error waiting for process: exit status 1
Error linting charts: Error building dependencies for chart 'airbyte => (version: "0.45.2", path: "charts/airbyte")': Error waiting for process: exit status 1

@xpuska513
Copy link
Contributor

It's unclear to me where the charts are actually stored, because changes in this code repo don't line up with the helm chart repo.

I'm also happy to run the helm-docs commands to update the README if they are run in ci/cd.

Charts from this repo are being packaged and then store in https://github.com/airbytehq/helm-charts repo
See this workflow for more details
For now the version mismatch was seen due to the action used in the workflow for generating semantic version be unstable from time to time(I'm looking for the root cause, but probably it's due to one PR was merged with non-semantic comit thus breaking the logic of action used + hardcoded values for version bump(commit flags))

@xpuska513
Copy link
Contributor

I have run chart-testing locally, and there are some additional changes it would like to see, around newlines in values.yaml, version pinning in https://github.com/airbytehq/airbyte/blob/master/charts/airbyte/Chart.yaml#L26.

I'll add chart-testing utility into a check workflow that will make sure everything's okay before approving PR(commit and PR have semantic format + cart-testing validation is successfull)

Thanks for pointing out!

@xpuska513 xpuska513 changed the title fix(charts): proplerly render extraContainers fix: proplerly render extraContainers Sep 12, 2022
@xpuska513
Copy link
Contributor

@bdashrad, everything LGTM, but before approving I'll clone your repo and deploy airbyte locally.

@bdashrad
Copy link
Contributor Author

Thanks @xpuska513, it looks like the chart version numbering has changed, so let me know what you'd like me to do there.

## requests:
## memory: 256Mi
## cpu: 250m
requests: {}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This aligns this section to be the same as the other charts in the repo.

Copy link
Contributor

@xpuska513 xpuska513 left a comment

Choose a reason for hiding this comment

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

LGTM
Thanks for your contribution!

@xpuska513 xpuska513 merged commit 1c2a851 into airbytehq:master Sep 13, 2022
@bdashrad bdashrad deleted the extraContainers-nindent branch September 13, 2022 12:59
robbinhan pushed a commit to robbinhan/airbyte that referenced this pull request Sep 29, 2022
* fix: proplerly render extraContainers

Signed-off-by: Brad Clark <bdashrad@gmail.com>

* fix: ignore charts path

Signed-off-by: Brad Clark <bdashrad@gmail.com>

* feat: add support for extraContainers and standardize to match other charts in repo

Signed-off-by: Brad Clark <bdashrad@gmail.com>

* fix: fix rendered chart indentation

Signed-off-by: Brad Clark <bdashrad@gmail.com>

Signed-off-by: Brad Clark <bdashrad@gmail.com>
jhammarstedt pushed a commit to jhammarstedt/airbyte that referenced this pull request Oct 31, 2022
* fix: proplerly render extraContainers

Signed-off-by: Brad Clark <bdashrad@gmail.com>

* fix: ignore charts path

Signed-off-by: Brad Clark <bdashrad@gmail.com>

* feat: add support for extraContainers and standardize to match other charts in repo

Signed-off-by: Brad Clark <bdashrad@gmail.com>

* fix: fix rendered chart indentation

Signed-off-by: Brad Clark <bdashrad@gmail.com>

Signed-off-by: Brad Clark <bdashrad@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/platform issues related to the platform community kubernetes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Helm chart incorrectly renders extraContainers
4 participants