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 item assignment issue with ConfigSecretVolume #924

Merged
merged 1 commit into from
Aug 14, 2023

Conversation

Molaire
Copy link
Contributor

@Molaire Molaire commented Aug 14, 2023

This PR fixes the schema assumption that configsecretVolume's "items" field is an array. I did not find the source of the array to tuple transformation, but this is the only place in the code that assume it's a writable array, so this also resolve the problem.

This test job was used with a locally modified version of tron in infrastage.

BEFORE change: https://fluffy.yelpcorp.com/i/7h1Tdwvblkf4KKKxz74V4T83XSbDxfsL.html
run successfully on infrastage: https://fluffy.yelpcorp.com/i/RWBcgXWqVfcsrFVXf9rtwfN6B9N7RpT4.html
retries successfully on infrastage: https://fluffy.yelpcorp.com/i/0d9hRM3hQ3KWv0nSDcHtTKGDLK7phXpt.html

The (currently) failing itest is also present for working commits in master. I don't know what this is about.

@Molaire Molaire force-pushed the u/vit/fix-configsecretvolume branch from a89ba10 to 32c99a7 Compare August 14, 2023 15:59
@Molaire Molaire marked this pull request as ready for review August 14, 2023 18:10
@Molaire Molaire changed the title Fix issue with ConfigSecretVolumeItem Fix issue with ConfigSecretVolume Aug 14, 2023
@Molaire Molaire changed the title Fix issue with ConfigSecretVolume Fix item assignment issue with ConfigSecretVolume Aug 14, 2023
Copy link
Member

@danielpops danielpops left a comment

Choose a reason for hiding this comment

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

lgtm

>>> l = list(('a', 'b'))
>>> l[1] = 'new'
>>> l
['a', 'new']

Copy link
Member

@nemacysts nemacysts left a comment

Choose a reason for hiding this comment

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

imo, we probably want a more descriptive comment for future yelpers - but lgtm otherwise :)

tron/config/schema.py Outdated Show resolved Hide resolved
@Molaire Molaire force-pushed the u/vit/fix-configsecretvolume branch from 4294210 to 9bf1c61 Compare August 14, 2023 20:29
@Molaire Molaire merged commit c60fef3 into master Aug 14, 2023
2 of 3 checks passed
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