MGMT-16066: Resource Subscription Server #107
Conversation
|
@irinamihai: This pull request references MGMT-16066 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the epic to target the "4.17.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
internal/service/constants.go
Outdated
| ) | ||
|
|
||
| const ( | ||
| SUBSCRPTION_TYPE_ALARM = "ALARM" |
There was a problem hiding this comment.
Should these be SubscriptionTypeAlarm and SubscriptionTypeInfrastructureInventory to be consistent with the typical naming for constants in Go?
| SetLogger(logger). | ||
| SetLoggingWrapper(loggingWrapper). | ||
| SetCloudID(cloudID). | ||
| SetExtensions(extensions...). | ||
| SetKubeClient(kubeClient). | ||
| SetSubscriptionType("ALARM"). |
There was a problem hiding this comment.
I wonder if the storage should be created outside of the handler and then passed as a parameter instead of forcing the subscription handler to know the type of subscription. Is there any difference in the behaviour other than the namespace/name of the configmap where the subscriptions are stored?
There was a problem hiding this comment.
I made the configuration map name and name spaces configurable in my current PR. but not committed.
There was a problem hiding this comment.
There are also 2 other instances where we need to translate the reponse. The Alarm and Infrastructure subscription responses differ a bit. In decodeSubId and encodeSubId. In these cases the handler needs to know the subscription type so that it translates correctly.
There was a problem hiding this comment.
@Jennifer-chen-rh , that's great. Would it be ok if we tried to merge the current PR first? Then yours will actually be shorter. The rebase shouldn't be a headache.
There was a problem hiding this comment.
Sure. Mine has more changes and comparatively harder to be a rebase code base.
| @@ -206,8 +206,8 @@ func (s *KubeConfigMapStore) ReadAllEntries(ctx context.Context) (result map[str | |||
|
|
|||
| func (s *KubeConfigMapStore) ProcessChanges(ctx context.Context, dataMap **map[string]data.Object, lock *sync.Mutex) (err error) { | |||
| raw_opt := metav1.SingleObject(metav1.ObjectMeta{ | |||
| Namespace: s.nameSpace, | |||
There was a problem hiding this comment.
I made similar change in my current notification server PR.
irinamihai
force-pushed
the
MGMT-16066-resource-sub-server
branch
from
b236434 to
7d13295
Compare
|
Looks good to me. |
| SetCloudID(cloudID). | ||
| SetExtensions(extensions...). | ||
| SetKubeClient(kubeClient). | ||
| SetSubscriptionType("INFRA-ENV"). |
There was a problem hiding this comment.
Should the SubscriptionTypeInfrastructureInventory constant be used here?
There was a problem hiding this comment.
You're right. Initially I saw they were in different packages, but I noticed the service package is already imported here, so I will use it. Thanks!
|
|
||
| err = nil | ||
|
|
||
| if h.persistStore.Name == AlarmConfigMapName { |
There was a problem hiding this comment.
nit: we can consider switch/case here for future expansion
| // get consumer name, subscriptions | ||
| err = h.jqTool.Evaluate( | ||
| `{ | ||
| "subscriptionId": $subId, |
There was a problem hiding this comment.
Just to keep the convention, maybe we can rename it to something like $resourceSubId
There was a problem hiding this comment.
Oh, I was actually referring to the name of the variable ($subId). It just looked a bit weird at first glance since it's generic compared to $alarmSubId. But feel free to ignore, it makes better sense now.
| jq.String("$subId", subId), | ||
| ) | ||
| } | ||
|
|
||
| if err != nil { | ||
| return |
There was a problem hiding this comment.
Do we want some log here? If not, seems lines 473-475 are redundant.
There was a problem hiding this comment.
I'll remove these.
| jq.String("$alarmSubId", subId), | ||
| ) | ||
|
|
||
| err = nil |
irinamihai
force-pushed
the
MGMT-16066-resource-sub-server
branch
from
7d13295 to
959a307
Compare
| err = h.jqTool.Evaluate( | ||
| `.alarmSubscriptionId`, input, &output) | ||
|
|
||
| err = nil |
There was a problem hiding this comment.
Same comments from encodeSubId - consider to apply also for this func.
Description: - update the alarm subscription handler to a generic subscription handler that can also be used for resource subscriptions - add a new command for the resource subscription server - update the alarm and resource subscription servers to also set the subscription type
irinamihai
force-pushed
the
MGMT-16066-resource-sub-server
branch
from
959a307 to
946666e
Compare
|
/lgtm |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: danielerez The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
Description:
oran-o2ims start infrastructure-inventory-subscription-server