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

Fixes bug when reading tile_name variable #482

Merged

Conversation

ryanwittrup
Copy link
Contributor

Our team recently experienced CI failures when trying to bake a tile, after passing in the tas variables file

That file has the properties:
tile_name: SRT

The result was a strange error: 'tile_name value must be a string got value "SRT" with type string'

After investigating more, we found this commit included a small typo in how the 'ok' value is handled

Worth noting - it was very hard to test this via the existing Bake tests. Because of this, we needed to export the function to be able to test it properly, but we know this is probably not ideal.

Our hope is you can help to review this and find a better way to test via the existing fixturing, or be able to confirm that making it public is acceptable

Exposes the BakeArgumentsFromKilnfileConfiguration to be public for
easier testing - we know this may not be ideal, and are open to changes
that use the existing Bake tests

[#187167974](https://www.pivotaltracker.com/story/show/187167974)

Authored-by: Ryan Wittrup <rwittrup@vmware.com>
@cf-gitbot
Copy link
Member

We have created an issue in Pivotal Tracker to manage this. Unfortunately, the Pivotal Tracker project is private so you may be unable to view the contents of the story.

The labels on this github issue will be updated when the story is started.

Copy link
Contributor

@crhntr crhntr left a comment

Choose a reason for hiding this comment

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

This looks good. I like the test boundary. Exporting that function is the right move.

Most of the tests are passing locally for me. There is a broken acceptance tests. I don't think the failure is related to this change. I feel okay merging. I'll rebase #481 on main once this is merged and dig into fixing the assertion there. It might already be fixed.

@crhntr crhntr merged commit 1c5134d into pivotal-cf:main Mar 7, 2024
2 of 3 checks passed
@crhntr
Copy link
Contributor

crhntr commented Mar 7, 2024

@pabloarodas and I paired on reviewing this and decided to merge.

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