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

Fix:adds network name key #3356

Merged
merged 6 commits into from
Oct 26, 2018
Merged

Conversation

adityatandon007
Copy link

Adds Network name key (WIP) related to #3336

Testing

Trade-offs

Dependencies

Screenshots

@mesosphere-ci
Copy link
Collaborator

Can one of the admins verify this patch?

2 similar comments
@mesosphere-ci
Copy link
Collaborator

Can one of the admins verify this patch?

@mesosphere-ci
Copy link
Collaborator

Can one of the admins verify this patch?

@adityatandon007
Copy link
Author

adityatandon007 commented Oct 10, 2018

@nLight you can now look into this and let me know the key for getting the network name in getNetworkName function to return

if (!networks || !networks.length) {
return null;
}
// need to get the networks to return here for network name
Copy link
Author

Choose a reason for hiding this comment

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

@nLight what is the key name that needs to be mapped to return the network name

@nLight
Copy link
Contributor

nLight commented Oct 10, 2018

@adityatandon007 thank you, I will have a look!

@nLight nLight self-assigned this Oct 10, 2018
@nLight
Copy link
Contributor

nLight commented Oct 11, 2018

Hey @adityatandon007, so the key name is name. But it's a bit deeper than that.
Host networks don't have names, only container networks do.
There could be multiple container networks. This is why we map networks to show Network Type.
Therefore we should map network names as well. Also let's use plural in the function name getNetworkName*s* so it matches with getNetworkTypes.

Thanks for dealing with this!

screenshot 2018-10-11 at 14 13 51

@adityatandon007
Copy link
Author

@nLight I didn't get the point.

@nLight
Copy link
Contributor

nLight commented Oct 11, 2018

@adityatandon007 the field name you are looking for called name and we going to need to show names for all networks.

if (!networks || !networks.length) {
return null;
}
// need to get the networks to return here for network name
Copy link
Contributor

Choose a reason for hiding this comment

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

How about this?

  if (networks[0].name) {
    return networks[0].name;
  }

Copy link
Author

Choose a reason for hiding this comment

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

@GeorgiSTodorov should I add this in my function. Has the network type changed to network mode

Copy link
Author

Choose a reason for hiding this comment

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

@GeorgiSTodorov should I add this in my function. Has the network type changed to network mode

Copy link
Author

Choose a reason for hiding this comment

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

@GeorgiSTodorov has the Network Type changed to Network Mode. Should I add the above snippet in my function to return network name

Copy link
Contributor

Choose a reason for hiding this comment

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

@adityatandon007
We should show all names not only the first one.
Changing Network type to Network Mode is not in scope of the ticket.

@GeorgiSTodorov
Copy link
Contributor

@nLight is changing Network Type to Network Mode in the scope of this ticket?

@adityatandon007
Copy link
Author

@GeorgiSTodorov has the Network Type changed to Network Name ? Should I add the above snippet as mentioned by you in comment section

@GeorgiSTodorov
Copy link
Contributor

@adityatandon007 It is changed in the jira screenshots, but I am not sure if chaning it is in scope of this ticket.

Adds Network names key for config section
@adityatandon007 adityatandon007 changed the title Fix [WIP]:adds network name key Fix:adds network name key Oct 24, 2018
return null;
}

return networks.map(({ name }) => NETWORK_MODE_NAME[name]).join(", ");
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe it should be just name without NETWORK_MODE_NAME[name]

Suggested change
return networks.map(({ name }) => NETWORK_MODE_NAME[name]).join(", ");
return networks.map(({ name }) => name).join(", ");

Adds Network names key for config section update
@nLight
Copy link
Contributor

nLight commented Oct 25, 2018

Thanks for the effort! Let's wait for CI and maybe @GeorgiSTodorov could give it a look 👀

@nLight
Copy link
Contributor

nLight commented Oct 25, 2018

To test multiple networks. Tradeoff - it won't start since UCR doesn't support dcos6but you will see that the fix is working.

{
  "id": "/pod",
  "version": "2018-10-25T14:55:31.424Z",
  "containers": [
    {
      "name": "container-1",
      "resources": {
        "cpus": 0.1,
        "mem": 128,
        "disk": 0,
        "gpus": 0
      },
      "endpoints": [
        {
          "name": "ping",
          "containerPort": 80,
          "networkNames": ["dcos"],
          "hostPort": 0,
          "protocol": [
            "tcp"
          ]
        }
      ],
      "image": {
        "kind": "DOCKER",
        "id": "nginx"
      }
    }
  ],
  "networks": [
    {
      "name": "dcos",
      "mode": "container"
    },
    {
      "name": "dcos6",
      "mode": "container"
    }
  ],
  "scaling": {
    "instances": 1,
    "kind": "fixed"
  },
  "scheduling": {
    "placement": {
      "constraints": []
    }
  },
  "executorResources": {
    "cpus": 0.1,
    "mem": 32,
    "disk": 10
  },
  "volumes": [],
  "fetch": []
}

@GeorgiSTodorov
Copy link
Contributor

@nLight ,this is what I get when I try to test your json:
screenshot from 2018-10-26 11-25-56

@nLight
Copy link
Contributor

nLight commented Oct 26, 2018

@GeorgiSTodorov I updated the json

@GeorgiSTodorov
Copy link
Contributor

Same thing happens again. "networkNames": ["dcos"] disappears when clicking Review & Run.

@nLight
Copy link
Contributor

nLight commented Oct 26, 2018

@GeorgiSTodorov please use JSON only mode, that's a known bug in the form

Copy link
Contributor

@GeorgiSTodorov GeorgiSTodorov left a comment

Choose a reason for hiding this comment

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

screenshot from 2018-10-26 11-39-45

Looks good. Thank you @adityatandon007 and @nLight .

@nLight nLight merged commit 4300ff0 into dcos:master Oct 26, 2018
@mesosphere-ci
Copy link
Collaborator

🎉 This PR is included in version 2.34.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants