search

ServiceNowITOM / servicenow-ansible

22 stars 28 forks source link

Version 1.0.5 broke backward compatibility #57

Closed tadeboro closed 3 years ago

tadeboro commented 3 years ago

The PR https://github.com/ServiceNowITOM/servicenow-ansible/pull/53 (or maybe was https://github.com/ServiceNowITOM/servicenow-ansible/pull/47 the source - it is hard to tell because this repo has a confusing branching strategy) contains backward-incompatible changes in the ServiceNow record modules.

The newly introduced auth option silently "degrades" the OAuth authentication users into using basic authentication.

Version pin PR for the Ansible package: https://github.com/ansible-community/ansible-build-data/pull/65

n3pjk commented 3 years ago

This was identified as a breaking upgrade made necessary to distinguish Basic, OAuth, OpenID, and any future authentication protocols. A patch can be added that, if auth is not specified but client_id is present, assume OAuth.

tadeboro commented 3 years ago

This was identified as a breaking upgrade made necessary to distinguish Basic, OAuth, OpenID, and any future authentication protocols. A patch can be added that, if auth is not specified but client_id is present, assume OAuth.

The problem is that this was released in a patch version. Collections should follow semantic versioning that requires a major version bump if the release contains backward-incompatible changes.

The right way forward here would probably be:

  1. Revert the change and release 1.0.6 that is backward-compatible.
  2. Reapply the change and release 2.0.0.

Alternatively (if the major version bump is not desired), the change can be reworked to simulate the 1.0.4 behavior. In this case, there is no need for 2.0.0.

/cc @willtome

n3pjk commented 3 years ago

I'm adding the back compatibility now using the v1.0.5 tag as the baseline. I am also restoring order_by functionality to snow_record_find. I think reverting and moving to 2.0.0 makes sense.

Sent from my Verizon, Samsung Galaxy smartphone Get Outlook for Androidhttps://aka.ms/AAb9ysg

From: Tadej Borovšak @.> Sent: Tuesday, April 27, 2021 6:59:35 PM To: ServiceNowITOM/servicenow-ansible @.> Cc: Paul Knight @.>; Comment @.> Subject: Re: [ServiceNowITOM/servicenow-ansible] Version 1.0.5 broke backward compatibility (#57)

This was identified as a breaking upgrade made necessary to distinguish Basic, OAuth, OpenID, and any future authentication protocols. A patch can be added that, if auth is not specified but client_id is present, assume OAuth.

The problem is that this was released in a patch version. Collections should follow semantic versioning that requires a major version bump if the release contains backward-incompatible changes.

The right way forward here would probably be:

  1. Revert the change and release 1.0.6 that is backward-compatible.
  2. Reapply the change and release 2.0.0.

Alternatively (if the major version bump is not desired), the change can be reworked to simulate the 1.0.4 behavior. In this case, there is no need for 2.0.0.

/cc @willtomehttps://github.com/willtome

— You are receiving this because you commented. Reply to this email directly, view it on GitHubhttps://github.com/ServiceNowITOM/servicenow-ansible/issues/57#issuecomment-828001807, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AA2LV67N25VRZB3EFWM7DYDTK46VPANCNFSM43FRIC5A.

amittell commented 3 years ago

@n3pjk does #59 provide backward compatibility?

If so, any reason to revert v1.0.5 and release v2.0.0 vs just releasing v1.0.6 with the backward compatibility changes? I think I prefer this route to v2.0.0 as there are no major functionality changes.

/cc @tadeboro for input

n3pjk commented 3 years ago

The compatibility fix is in, so going to 1.0.6 works.

Sent from my Verizon, Samsung Galaxy smartphone Get Outlook for Androidhttps://aka.ms/AAb9ysg

From: amittell @.> Sent: Wednesday, April 28, 2021 4:01:22 PM To: ServiceNowITOM/servicenow-ansible @.> Cc: Paul Knight @.>; Mention @.> Subject: Re: [ServiceNowITOM/servicenow-ansible] Version 1.0.5 broke backward compatibility (#57)

@n3pjkhttps://github.com/n3pjk does #59https://github.com/ServiceNowITOM/servicenow-ansible/pull/59 provide backward compatibility?

If so, any reason to revert v1.0.5 and release v2.0.0 vs just releasing v1.0.6 with the backward compatibility changes? I think I prefer this route to v2.0.0 as there are no major functionality changes.

/cc @tadeborohttps://github.com/tadeboro for input

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/ServiceNowITOM/servicenow-ansible/issues/57#issuecomment-828739259, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AA2LV6YAVEVPI4AZH6CZ2ZTTLBSRFANCNFSM43FRIC5A.

n3pjk commented 3 years ago

Yes, #59 provides the backward compatibility. We can just release 1.0.6. We might want to note that 1.0.5 has a compatibility issue, though the issue is already in the 1.0.5 changelog as a break.

Sent from my Verizon, Samsung Galaxy smartphone Get Outlook for Androidhttps://aka.ms/AAb9ysg

From: amittell @.> Sent: Wednesday, April 28, 2021 4:01:22 PM To: ServiceNowITOM/servicenow-ansible @.> Cc: Paul Knight @.>; Mention @.> Subject: Re: [ServiceNowITOM/servicenow-ansible] Version 1.0.5 broke backward compatibility (#57)

@n3pjkhttps://github.com/n3pjk does #59https://github.com/ServiceNowITOM/servicenow-ansible/pull/59 provide backward compatibility?

If so, any reason to revert v1.0.5 and release v2.0.0 vs just releasing v1.0.6 with the backward compatibility changes? I think I prefer this route to v2.0.0 as there are no major functionality changes.

/cc @tadeborohttps://github.com/tadeboro for input

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/ServiceNowITOM/servicenow-ansible/issues/57#issuecomment-828739259, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AA2LV6YAVEVPI4AZH6CZ2ZTTLBSRFANCNFSM43FRIC5A.