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

accept JSON includes #2265

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

accept JSON includes #2265

wants to merge 2 commits into from

Conversation

spoltier
Copy link

#2201 disabled using JSON as part of a bundle definition. I believe this was not intended.

Changes

Accept json files as includes, just as YAML files.

Tests

Covered by the tests in #2201

databricks#2201 disabled using JSON as part of a bundle definition. I believe this was not intended.
@andrewnester
Copy link
Contributor

@spoltier could you please share what's your use case for including JSONs?

@spoltier
Copy link
Author

We have JSON job definitions originally copied and lightly adapted JSONs from the jobs UI, which works just as well as YAML to define resources. Since the validation is about preventing arbitrary file includes, I assumed it was an oversight.
Do the maintainer plan to remove JSON support for bundle conf? If so it would perhaps earn a minor release (as opposed to patch)

@andrewnester
Copy link
Contributor

We expected that some users might use JSON for defining configuration but since YAML is also available from UI we did not expect it to be very common. There is no plan to drop JSON support for config, so we will bring it back, thanks for reporting!

As to PR itself, to accept the changes it requires new contributors to sign CLA, if that's something you're willing to do, feel free to reach out to [email protected] and we will take it from there.

@spoltier
Copy link
Author

We expected that some users might use JSON for defining configuration but since YAML is also available from UI we did not expect it to be very common. There is no plan to drop JSON support for config, so we will bring it back, thanks for reporting!

As to PR itself, to accept the changes it requires new contributors to sign CLA, if that's something you're willing to do, feel free to reach out to [email protected] and we will take it from there.

Thanks! I've sent an email.

diags = diags.Append(diag.Diagnostic{
Severity: diag.Error,
Summary: "Files in the 'include' configuration section must be YAML files.",
Copy link
Contributor

Choose a reason for hiding this comment

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

You also need to change an output for the tests here to make sure it matches: https://github.com/databricks/cli/blob/main/acceptance/bundle/includes/non_yaml_in_include/output.txt

Copy link
Author

Choose a reason for hiding this comment

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

thank you, changed the output

Copy link

An authorized user can trigger integration tests manually by following the instructions below:

Trigger:
go/deco-tests-run/cli

Inputs:

  • PR number: 2265
  • Commit SHA: 826486c9f237119be366d2d47a0a49d2d9b867e0

Checks will be approved automatically on success.

@andrewnester andrewnester self-requested a review January 30, 2025 09:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants