Bug 488992 - Duplicate API channel.software.subscribeSystem and channel.software.setSystemChannels
Duplicate API channel.software.subscribeSystem and channel.software.setSystem...
Status: CLOSED CURRENTRELEASE
Product: Red Hat Satellite 5
Classification: Red Hat
Component: API (Show other bugs)
530
All Linux
low Severity medium
: ---
: ---
Assigned To: Brad Buckingham
Sayli Karmarkar
:
Depends On:
Blocks: 456996
  Show dependency treegraph
 
Reported: 2009-03-06 11:49 EST by Sayli Karmarkar
Modified: 2015-03-22 21:09 EDT (History)
2 users (show)

See Also:
Fixed In Version: sat530
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-09-10 15:54:44 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)

  None (edit)
Description Sayli Karmarkar 2009-03-06 11:49:58 EST
Description of problem:
Duplicate API channel.software.subscribeSystem and channel.software.setSystemChannels

Expected results:
I think setSystemChannels makes lot of sense than subscribeSystem as its not confusing and we also have similar other API like listSystemChannels.
Comment 1 Brad Buckingham 2009-03-06 16:50:08 EST
Just did a bit of looking at the code and digging for some history to better understand why we might have this duplication... And this may have opened a 'can of worms' :), but something I do feel worth addressing.

It looks like setSystemChannels was added first and later subscribeSystems.  While the descriptions make the API behaviors sound the same, there are some differences.

Primary differences observed:

- channel.software.setSystemChannels 

  - requires user to provide a full list of channels to be assigned to the system... for example, they cannot provide just a child channel to add to their current settings
  - does not provide ability to 'clear' the system's channel subscriptions.. for example, user cannot pass in [''] for the list of channels to remove channel subscriptions

- channel.software.subscribeSystem

  - allows user to modify their current channel subscriptions... for example, they can add a child channel w/o actually repeating their base channel in the input
  - allows the user to clear the system's channel subscriptions... 
  - does not allow the user an 'unsubscribe' option aside from clearing the system's channel subscriptions

That said, I prefer the logic provided by subscribeSystem because it allows more flexibility in the behavior; however, it appears that we have some additional duplication in the APIs.  For example:

1. system.setBaseChannel
2. system.setChildChannels

These APIs provide similar functionality; however, they require the user to provide a channel ID as an input instead of a label.  Unfortunately, use of channel ID is inconsistent with how channels are typically handled/accessed (e.g. channel.software APIs use channel label)

All that said, the following is my recommendation:

1. deprecate channel.software.subscribeSystem
2. deprecate channel.software.setSystemChannels
3. deprecate system.setBaseChannel(using channel ids)
4. deprecate system.setChildChannels(user channel ids)

5. create system.setBaseChannel(using channel labels)
6. create system.setChildChannels(using channel labels)

The intent is to make this functionality a more consistent and avoid the duplication.  It's not a huge amount of work, but since it increases the scope, please let me know if it is Ok to proceed with this in the current release.
Comment 2 Sayli Karmarkar 2009-03-06 17:00:48 EST
+1. I also saw APIs 1 to 4 when creating automation tests and for no good reason they have similar yet a bit different functionality. I think implementing 5 and 6 is much cleaner approach. 

(In reply to comment #1)
> All that said, the following is my recommendation:
> 
> 1. deprecate channel.software.subscribeSystem
> 2. deprecate channel.software.setSystemChannels
> 3. deprecate system.setBaseChannel(using channel ids)
> 4. deprecate system.setChildChannels(user channel ids)
> 
> 5. create system.setBaseChannel(using channel labels)
> 6. create system.setChildChannels(using channel labels)
> 
> The intent is to make this functionality a more consistent and avoid the
> duplication.  It's not a huge amount of work, but since it increases the scope,
> please let me know if it is Ok to proceed with this in the current release.
Comment 3 Brandon Perkins 2009-03-09 00:02:15 EDT
go4it
Comment 4 Brad Buckingham 2009-03-13 17:09:18 EDT
git commit: 02da57018dae1c7f211e85f6e7da740cd1e45997

Summary of modifications;

- deprecate channel.software.subscribeSystem
- deprecate channel.software.setSystemChannels
- deprecate system.setBaseChannel(string sessionKey, int systemId, int channelId)

- added system.setBaseChannel(string sessionKey, int systemId, string channelLabel)

- modified system.setChildChannels(string sessionKey, int systemId, array[channelId]) to be system.setChildChannels(string sessionKey, int systemId, array[channelIdOrLabel]) where the array may contain either a list of channel ids (int) or a list of channel labels (string).  The support for the list of channel ids is being deprecated.   (Due to the way Java behaves for List objects, it was not possible to deprecate the entire API because a List is just a container of objects and it doesn't differentiate at compile time a List<String> vs a List<Integer>.)
Comment 5 Brad Buckingham 2009-03-23 16:28:21 EDT
mass move to ON_QA
Comment 6 Sayli Karmarkar 2009-03-24 13:54:44 EDT
Verified. Added api tests for new setChildChannels and setBaseChannel and also for deprecated subscribeSystem.
Comment 7 Steve Salevan 2009-08-26 14:29:00 EDT
Moving to RELEASE_PENDING from latest Stage build; verified that deprecated methods were deprecated and that non-deprecated methods worked as specified.
Comment 8 Brandon Perkins 2009-09-10 15:54:44 EDT
An advisory has been issued which should help the problem
described in this bug report. This report is therefore being
closed with a resolution of ERRATA. For more information
on therefore solution and/or where to find the updated files,
please follow the link below. You may reopen this bug report
if the solution does not work for you.

http://rhn.redhat.com/errata/RHEA-2009-1434.html

Note You need to log in before you can comment on or make changes to this bug.