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

Adding mesh gateway, secret, secret value, network, code package and network commands(GET, LIST, DELETE) #141

Merged
merged 14 commits into from
Oct 20, 2018

Conversation

jeffj6123
Copy link
Member

Fixes # .

Changes include:
GET/LIST/DELETE Commands for gateway, secret/secret value, network, and code package(Only get)

Verified:

  • Build and test CI passes
  • History and readme updated to reflect changes
  • Package version updated according to semantic versioning rules
  • Tests modified or added, when applicable
  • Updated code owners file, when applicable
  • Read the PR checklist

@@ -14,7 +14,7 @@
from knack.commands import CLICommandsLoader, CommandGroup
from knack.help import CLIHelp
from sfctl.apiclient import create as client_create
from sfctl.apiclient import mesh_app_create, mesh_volume_create, mesh_service_create, mesh_service_replica_create #pylint: disable=line-too-long
from sfctl.apiclient import mesh_app_create, mesh_volume_create, mesh_service_create, mesh_service_replica_create, mesh_network_create, mesh_code_package_create, mesh_gateway_create, mesh_secret_create, mesh_secret_value_create #pylint: disable=line-too-long
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we break this up into multiple lines?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes.

src/sfctl/commands.py Show resolved Hide resolved
src/sfctl/commands.py Show resolved Hide resolved


def get_secret_value(client, secret_resource_name, secret_value_resource_name, show_value):
"""structure is meant to make testing easier because testing
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you clarify what is referred to by structure?

Copy link
Member Author

Choose a reason for hiding this comment

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

This might not be necessary to say actually, more that on mesh cli it looks like
secret_data = client.get(resource_group_name, secret_name, secret_value_resource_name)
if show_value:
secret_value = client.list_value(resource_group_name, secret_name, secret_value_resource_name)
secret_data.value = secret_value['value']
return secret_data

Which looks simpler but always makes the first request for the general secret info and then an optional request to get the value. But because with how the current generated testing library is set up and only checks against the first request made, by slightly moving it around can check the show value scenario also.

@coveralls
Copy link

coveralls commented Oct 19, 2018

Coverage Status

Coverage increased (+0.5%) to 83.164% when pulling ed23c5a on jejarry-mesh-commands into f25e471 on master.


helps['mesh code-package'] = """
type: group
short-summary: Gets the logs for the container of the specified code package of the service replica.
Copy link
Contributor

Choose a reason for hiding this comment

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

Gets -> Get

Copy link
Contributor

Choose a reason for hiding this comment

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

of the service replica -> for the given service replica

changing up the wording - suggestion only


helps['mesh secretvalue'] = """
type: group
short-summary: Get and delete mesh secret value resources
Copy link
Contributor

Choose a reason for hiding this comment

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

secret value -> secretvalue

helps['mesh secretvalue show'] = """
type: command
short-summary: Retrieve the value of a specified version of a secret resource
long-summary: Retrieve the value of a specified version of a secret resource.
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't repeat the short summary in the long summary. Users will see it repeated right next to each other.

type: command
short-summary: Retrieve the value of a specified version of a secret resource
long-summary: Retrieve the value of a specified version of a secret resource.
Use the --show-value flag to see the actual value
Copy link
Contributor

Choose a reason for hiding this comment

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

I would leave the parameter descriptions out unless you feel that it would be very beneficial for the users to see this info here.

package to
- name: --show-value
type: bool
short-summary: Show file upload progress for large packages
Copy link
Contributor

Choose a reason for hiding this comment

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

Content is incorrect

short-summary: The name of the secret resource.
- name: --secret-value-resource-name
type: string
short-summary: Version identifier of the secret value
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not too familiar with this... would it be very obvious to the user where the version would be?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, when getting any information about a secret value you would know which secret resource/secret value you want specifically. List secretvalue would let you look for potential secret values of a secret resource.

with ArgumentsContext(self, 'mesh secretvalue show') as arg_context:
arg_context.argument('secret_resource_name')
arg_context.argument('secret_value_resource_name')
arg_context.argument('show_value', required=False)
Copy link
Contributor

Choose a reason for hiding this comment

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

don't need to add required=false. It should be automatic because you provide a default value in the function signature - can you double check pls? thanks!

@Christina-Kang Christina-Kang merged commit 6e0a90b into master Oct 20, 2018
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

3 participants