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

Add dyn.Time to box a timestamp with its original string value #1732

Merged
merged 2 commits into from
Aug 29, 2024

Conversation

pietern
Copy link
Contributor

@pietern pietern commented Aug 29, 2024

Changes

If not explicitly quoted, the YAML loader interprets a value like 2024-08-29 as a timestamp. Such a value is usually intended to be a string instead. Our normalization logic was not able to turn a time value back into the original string.

This change boxes the time value to include its original string representation. Normalization of one of these values into a string can now use the original input value.

Tests

Unit tests in libs/dyn/convert.

libs/dyn/time.go Outdated
}
}

return Time{}, false
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we log a message here saying that format that was used is not supported?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, done. It now returns an error that says as much.

@pietern pietern enabled auto-merge August 29, 2024 12:56
@pietern pietern added this pull request to the merge queue Aug 29, 2024
Merged via the queue into main with commit 0f4891f Aug 29, 2024
5 checks passed
@pietern pietern deleted the dyn-box-time branch August 29, 2024 13:10
andrewnester added a commit that referenced this pull request Sep 5, 2024
CLI:
 * Do not error if we cannot prompt for a profile in `auth login` ([#1745](#1745)).

Bundles:
 * Pass along $AZURE_CONFIG_FILE to Terraform process ([#1734](#1734)).
 * Add prompt when a pipeline recreation happens ([#1672](#1672)).
 * Use materialized views in the default-sql template ([#1709](#1709)).
 * Update templates to latest LTS DBR ([#1715](#1715)).
 * Make lock optional in the JSON schema ([#1738](#1738)).
 * Do not suppress normalisation diagnostics for resolving variables ([#1740](#1740)).
 * Include a permissions section in all templates ([#1713](#1713)).
 * Fixed complex variables are not being correctly merged from include files ([#1746](#1746)).
 * Fixed variable override in target with full variable syntax ([#1749](#1749)).

Internal:
 * Consider serverless clusters as compatible for Python wheel tasks ([#1733](#1733)).
 * PythonMutator: explain missing package error ([#1736](#1736)).
 * Add `dyn.Time` to box a timestamp with its original string value ([#1732](#1732)).
 * Fix streaming of stdout, stdin, stderr in cobra test runner ([#1742](#1742)).

Dependency updates:
 * Bump github.com/Masterminds/semver/v3 from 3.2.1 to 3.3.0 ([#1741](#1741)).
github-merge-queue bot pushed a commit that referenced this pull request Sep 5, 2024
CLI:
* Do not error if we cannot prompt for a profile in `auth login`
([#1745](#1745)).

Bundles:

As of this release CLI will show a prompt is if there are configuration
changes which will lead to a DLT recreation.
Users can skip the prompt by specifying the `--auto-approve` flag

* Pass along $AZURE_CONFIG_FILE to Terraform process
([#1734](#1734)).
* Add prompt when a pipeline recreation happens
([#1672](#1672)).
* Use materialized views in the default-sql template
([#1709](#1709)).
* Update templates to latest LTS DBR
([#1715](#1715)).
* Make lock optional in the JSON schema
([#1738](#1738)).
* Do not suppress normalisation diagnostics for resolving variables
([#1740](#1740)).
* Include a permissions section in all templates
([#1713](#1713)).
* Fixed complex variables are not being correctly merged from include
files ([#1746](#1746)).
* Fixed variable override in target with full variable syntax
([#1749](#1749)).

Internal:
* Consider serverless clusters as compatible for Python wheel tasks
([#1733](#1733)).
* PythonMutator: explain missing package error
([#1736](#1736)).
* Add `dyn.Time` to box a timestamp with its original string value
([#1732](#1732)).
* Fix streaming of stdout, stdin, stderr in cobra test runner
([#1742](#1742)).

Dependency updates:
* Bump github.com/Masterminds/semver/v3 from 3.2.1 to 3.3.0
([#1741](#1741)).

---------

Co-authored-by: Pieter Noordhuis <pieter.noordhuis@databricks.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants