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

sanity: fix false negative sanity check version_added module #52806

Open
wants to merge 6 commits into
base: devel
from

Conversation

Projects
None yet
5 participants
@resmo
Copy link
Member

resmo commented Feb 22, 2019

SUMMARY

changed a non string version_added to string.

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

sanity

ADDITIONAL INFORMATION

@resmo

This comment has been minimized.

Copy link
Member Author

resmo commented Feb 22, 2019

bot_skip

@resmo resmo force-pushed the resmo:tmp/sanity-tests branch 5 times, most recently from 1ffcb3d to 975652f Feb 22, 2019

@resmo resmo changed the title [WIP] fix false negative sanity check sanity: fix false negative sanity check version_added module Feb 22, 2019

@resmo resmo requested a review from sivel Feb 22, 2019

@dagwieers dagwieers force-pushed the resmo:tmp/sanity-tests branch from 692eb36 to b2f54d8 Feb 22, 2019

@dagwieers dagwieers force-pushed the resmo:tmp/sanity-tests branch 2 times, most recently from a050986 to b526452 Feb 22, 2019

@dagwieers dagwieers force-pushed the resmo:tmp/sanity-tests branch from b526452 to acb0567 Feb 22, 2019

@dagwieers
Copy link
Member

dagwieers left a comment

LGTM, should be merged ASAP to avoid any merge conflicts.

The existing CI failures are fixes to the version.

lib/ansible/modules/cloud/lxc/lxc_container.py:0:0: E307 version_added should be '1.8.0'. Currently '1.8'
lib/ansible/modules/net_tools/netcup_dns.py:0:0: E307 version_added should be '2.7.0'. Currently '2.7'
@resmo

This comment has been minimized.

Copy link
Member Author

resmo commented Feb 22, 2019

Nice!

@felixfontein
Copy link
Contributor

felixfontein left a comment

I really like this! version_added should always be a string.

@gundalow
Copy link
Contributor

gundalow left a comment

Please don't include the change to test/sanity/ in this PR.

PR1: Fix all existing modules
PR2: Update Docs
PR3: Enforce via CI

@dagwieers

This comment has been minimized.

Copy link
Member

dagwieers commented Feb 22, 2019

@gundalow As you wish, I don't agree, as it makes it less clear.

@dagwieers

This comment has been minimized.

Copy link
Member

dagwieers commented Feb 23, 2019

Context

(17:00:57) dag: gundalow: https://github.com/ansible/ansible/pull/52806
(17:01:20) dag: gundalow: better not let this one sit, because of conflict-risk
(17:08:47) resmo: gundalow: also here, would appreciate fast track,
(17:10:57) sivel: my personal opinion, is that we need to vote in at least one of the IRC meetings whether we actually want to change the schema
(17:17:00) resmo: sivel: imho the convention was always "use string to not use float in versions" but was not enforced
(17:18:46) resmo: sivel: currently I get this https://github.com/ansible/ansible/pull/52683#issuecomment-466310921 which is really weird.
(17:19:04) resmo: the santiy check enforces to use float (...)
(17:19:24) resmo: for existing modules
(17:19:29) sivel: resmo: I made one check already, I may have missed another location.  We should just fix the check
(17:19:49) sivel: The previous fix just cast the values to str before comparing
(17:20:08) sivel: that was the decision the core team agreed upon when I made the change
(17:20:27) sivel: don't change the schema, just change the check to not consider type changes as bad.  Cast to str in validate-modules to check
(17:21:32) sivel: I'll duplciate my fix for E309 to E307 also, and merge it soon
(17:22:52) sivel: PR will be up soon
(17:23:12) resmo: sivel: I "fixed" all existing modules' verison_added (module one, not params)
(17:27:11) sivel: I think you are doing a lot of unnecessary busy work.  We have decided currently not to enforce version_added to be a string.  We could vote to change that, but the only change that needs to be done is just for valdiate-modules
(17:27:13) resmo: sivel: then like https://github.com/ansible/ansible/pull/52828 ?
(17:28:05) sivel: I don't think that is necessary at all.  Just a 1 line change to validate-modules to cast to string before comparing.  I have the PR, waiting for CI to merge
(17:34:50) resmo: sivel: and then I look at https://docs.ansible.com/ansible/latest/dev_guide/developing_modules_documenting.html#documentation-block and there we have "...This is a string, and not a float..."
(17:38:27) sivel: the schema is the source of truth, the docs are wrong
(17:38:48) felixfontein: floats for version numbers are really dangerous
(17:38:57) sivel: not that we cannot change it, but it shouldn't be decided unilaterally, it should be voted upon
(17:39:26) sivel: I'm sure it will get weird when we go to `2.10` if not a string :)
(17:39:37) felixfontein: yep, that's the main reason why it shouldn't be a float :)
(17:40:05) felixfontein: (or why lexicographic string comparison for version number strings sucks)
(17:41:22) felixfontein: there are multiple   removed_in_version=2.10   in the modules (various gitlab_* modules)
(17:44:21) resmo: I have a PR, merge it or close it. floats for version are simply wrong, versions are not nubmers and not floats.
(17:46:49) sivel: a square is a rectangle, however not every rectangle is a square
(17:49:48) felixfontein: sivel: would it be ok for you to at least change all existing version_added to str?
(17:50:23) sivel: I likely won't do it, I see resmo is submitting PRs to do so. I don't know that I have the time currently to review such large PRs.
(18:27:47) gundalow: dag: resmo sivel
(18:27:47) gundalow: Please don't include the change to `test/sanity/` in this PR.
(18:27:47) gundalow: PR1: Fix all existing modules - I don't see any reason why this can't be merged
(18:27:47) gundalow: If after vote:
(18:27:47) gundalow: PR2: Update Docs
(18:27:47) gundalow: PR3: Enforce via CI
(18:28:31) sivel: gundalow: I've separately submitted a fix for E307 to not fail on the type change, to mirror E309
(18:28:36) sivel: so that isn't holding PRs up any longer
(18:28:51) sivel: gundalow: I agree with your steps
@gundalow

This comment has been minimized.

Copy link
Contributor

gundalow commented Feb 25, 2019

CI failure is

lib/ansible/modules/cloud/lxc/lxc_container.py:0:0: E307 version_added should be '1.8.0'. Currently '1.8'
lib/ansible/modules/net_tools/netcup_dns.py:0:0: E307 version_added should be '2.7.0'. Currently '2.7'

Which can be ignored and will not exist after merge

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.