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

MGMT-6897: Swagger support multiple networks #2339

Merged

Conversation

YuviGold
Copy link
Contributor

@YuviGold YuviGold commented Aug 5, 2021

Assisted Pull Request

Description

The most straight-forward solution for maintaining multiple networks
is to have different tables for each one.

Then, there's no need to handle BLOBs marshalling and it avoid any
assumptions for order, amount, etc (Those would be validated as part of
the service logic).

Updating the networks would work the same as it goes for the monitored
operators.

  • In case the network weren't given (nil) then its update is ignored.
  • In case networks were provided - they are overriding the current
    cluster networks.

List all the issues related to this PR

  • New Feature

What environments does this code impact?

  • Cloud

How was this code tested?

  • No tests needed

Assignees

/cc @mkowalski
/cc @ori-amizur
/cc @eranco74

Checklist

  • Title and description added to both, commit and PR.
  • Relevant issues have been associated (see CONTRIBUTING guide)
  • Reviewers have been listed
  • This change does not require a documentation update (docstring, docs, README, etc)
  • Does this change include unit-tests (note that code changes require unit-tests)

Reviewers Checklist

  • Are the title and description (in both PR and commit) meaningful and clear?
  • Is there a bug required (and linked) for this change?
  • Should this PR be backported?

@openshift-ci openshift-ci bot added the api-review Categorizes an issue or PR as actively needing an API review. label Aug 5, 2021
@openshift-ci
Copy link

openshift-ci bot commented Aug 5, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: YuviGold

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Aug 5, 2021
swagger.yaml Outdated Show resolved Hide resolved
@YuviGold YuviGold mentioned this pull request Aug 5, 2021
23 tasks
@mkowalski
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 5, 2021
swagger.yaml Show resolved Hide resolved
swagger.yaml Show resolved Hide resolved
swagger.yaml Show resolved Hide resolved
description: Cluster networks that are associated with this cluster.
items:
type: object
$ref: '#/definitions/cluster_network'
Copy link
Contributor

Choose a reason for hiding this comment

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

What should be the logic for update? If a record exists - is it add or update? If it does not exist - is it delete?

Copy link
Contributor

Choose a reason for hiding this comment

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

Good question.
This is used in PATCH API - meaning the user doesn't have to pass the complete entity.

@avishayt thoughts on this?

Copy link
Contributor

Choose a reason for hiding this comment

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

The user doesn't need to pass the entire cluster resource, but the simplest way to implement this is that they pass the entire list of networks and we replace any previous entry. Let's not overcomplicate it.

Copy link
Contributor Author

@YuviGold YuviGold Aug 8, 2021

Choose a reason for hiding this comment

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

Updating the networks would work the same as it goes for the monitored operators.

  • In case the network weren't given (nil) then its update is ignored.
  • In case networks were provided - they are overriding the current
    cluster networks.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@mkowalski
Copy link
Contributor

/lgtm cancel
I think Ori's concerns are perfectly valid here

@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Aug 5, 2021
@openshift openshift deleted a comment from ori-amizur Aug 8, 2021
@openshift openshift deleted a comment from ori-amizur Aug 8, 2021
@openshift openshift deleted a comment from ori-amizur Aug 8, 2021
@openshift openshift deleted a comment from ori-amizur Aug 8, 2021
@openshift openshift deleted a comment from ori-amizur Aug 8, 2021
@openshift openshift deleted a comment from ori-amizur Aug 8, 2021
The most straight-forward solution for maintaining multiple networks
is to have different tables for each one.

Then, there's no need to handle BLOBs marshalling and it avoid any
assumptions for order, amount, etc (Those would be validated as part of
the service logic).

Updating the networks would work the same as it goes for the monitored
operators.
- In case the network weren't given (nil) then its update is ignored.
- In case networks were provided - they are overriding the current
  cluster networks.
@YuviGold YuviGold force-pushed the MGMT-6897-multiple-networks branch from 8930ad3 to 6e55db1 Compare August 8, 2021 15:15
@ori-amizur
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 8, 2021
@openshift-ci openshift-ci bot merged commit 196700b into openshift:master Aug 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-review Categorizes an issue or PR as actively needing an API review. approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants