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

feat: move custom variables to $(custom:...) with backward compatibility #2868

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

krocheck
Copy link
Member

@krocheck krocheck commented May 18, 2024

Responds, in part to #2812 by moving custom variables from $(internal:custom_...) to $(custom:...). Checks are added to maintain backwards compatibility with legacy names.

If we want to move forward I'll get the rest of the action items executed.

My main reason for attempting this is that with the addition of $(this:...), it seems logical to me to make this change. The fact that custom isn't a reserved word in v2.x is concerning, but I think is quite the edge case for someone to completely rename an instance to custom. But that can be investigated for upgrade mitigation ... though scrubbing the variables might become difficult.

TODO:

  • Validate basic functionality and backwards compatibility
  • Validate custom variable internal actions
  • Validate triggers
  • Update documentation
  • Update any labels, like indicated in Improve variable naming consistency #2812
  • Investigate custom instance name mitigation in v2-v3 upgrade

@krocheck krocheck marked this pull request as draft May 18, 2024 17:33
@Julusian
Copy link
Member

Julusian commented May 20, 2024

I have no problem with this being done.

Perhaps for a release or two (or forever?) we should keep the internal:custom_ around for compatibility? But without the definition, so that they won't show up in the ui.
They could be implemented so that internal:custom_X has a value of $(custom:X), to simplify setting the values (we use this 'trick' elsewhere, to get the $(this:page_name) and $(this:step) to be correctly reactive)

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.

2 participants