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

Use version strings instead of floats in examples #4332

Closed
wants to merge 19 commits into from

Conversation

blag
Copy link
Contributor

@blag blag commented Sep 6, 2018

The spec already accepts both YAML strings and floats in the version key, but it's probably a good idea to use version strings in our examples (which are also used in our documentation) because it's problematic to compare strings and floats in Python:

>>> '1.1' > 1.0  # Fine
True
>>> '0.9' > 1.0
True
>>> '1.0.1' > 2.0
True
>>> '1.1.0' > 2.0
True

and we can't use floats everywhere because we can't convert bugfix releases to floats:

>>> float('1.0.1')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ValueError: invalid literal for float(): 1.0.1

@m4dcoder m4dcoder self-requested a review September 6, 2018 19:00
Copy link
Contributor

@m4dcoder m4dcoder left a comment

Choose a reason for hiding this comment

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

Can you start with changes in the orquesta repo? I would like to see the required schema changes in the spec first. Also, if this double quotes is necessary, then you'll have to update all orquesta tests in the st2tests directory. The ones you updated here is only a fraction.

@blag
Copy link
Contributor Author

blag commented Sep 6, 2018

@m4dcoder We don't need to change anything in Orquesta - I've confirmed it already accepts version strings as well as unquoted floats (see my link in the summary). This change is simply changing our examples to follow what is probably best practices. And YAML is smart enough to interpret 1.0.1 as a string, so everything works as expected.

I can add a test that specifically tests version strings if you like, but I'm not too worried about this coverage.

@m4dcoder
Copy link
Contributor

m4dcoder commented Sep 6, 2018

So if the schema in orquesta supports float and string and there hasn't been any problem with the versioning and executing any orquesta workflow, what's the purpose of this change?

@blag
Copy link
Contributor Author

blag commented Sep 7, 2018

The thrust of this PR is subtle - it's simply about training users. Versions are fundamentally strings, not floats, and can only be compared as floats when neither version has a bugfix number. So this is nudging our users to use and think about versions as strings. Furthermore, Python can natively correctly compare version strings, but cannot reliably correctly compare version strings to floats.

Our current code does try to convert the version to a float, which works - for now - because the version can be thought of as a float. And if/when we do ever need to add a bugfix version to our accepted versions, we can adjust the code as necessary. So I don't think there's a need to change any of the Orquesta validation code right now, but I do think it would make our lives slightly easier to demonstrate versions as strings in our example workflow YAML.

And just to be clear: this isn't a huge deal if it doesn't get merged, it's just a tiny amount of easy polish.

@m4dcoder
Copy link
Contributor

Hey, I forgotten about this PR. So, let's close this when you have a chance. Please normalize the version to string in all the orquesta workflow examples and test fixtures. Make sure you update all the orquesta workflows under https://github.com/StackStorm/st2/tree/master/contrib/examples/actions/workflows and https://github.com/StackStorm/st2/tree/master/st2tests/st2tests/fixtures/packs/orquesta_tests/actions/workflows. Thanks.

@blag blag requested a review from m4dcoder April 5, 2019 05:30
@blag
Copy link
Contributor Author

blag commented Apr 19, 2019

@m4dcoder I fixed everything you brought up in your review. Please re-review at some point (doesn't have to be now).

Copy link
Contributor

@m4dcoder m4dcoder left a comment

Choose a reason for hiding this comment

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

Just make sure if there are any new examples/tests are added since this PR are also added in this update.

@stale
Copy link

stale bot commented Sep 18, 2019

Thanks for contributing to this issue. As it has been 90 days since the last activity, we are automatically marking is as stale. If this issue is not relevant or applicable anymore (problem has been fixed in a new version or similar), please close the issue or let us know so we can close it. On the contrary, if the issue is still relevant, there is nothing you need to do, but if you have any additional details or context which would help us when working on this issue, please include it as a comment to this issue.

@stale stale bot added the stale label Sep 18, 2019
@stale stale bot removed the stale label Dec 13, 2019
@blag blag closed this Feb 28, 2020
@arm4b arm4b deleted the use-version-strings-in-examples branch February 28, 2020 21:00
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.

None yet

2 participants