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

PR review for error messages #22

Open
wants to merge 8 commits into
base: eni
Choose a base branch
from
Open

PR review for error messages #22

wants to merge 8 commits into from

Conversation

sameshai
Copy link
Member

No description provided.

Signed-off-by: Sameer Shaikh <[email protected]>

ENI support

Signed-off-by: Sameer Shaikh <[email protected]>

ENI support

Signed-off-by: Sameer Shaikh <[email protected]>

ENI support

Signed-off-by: Sameer Shaikh <[email protected]>

ENI support

Signed-off-by: Sameer Shaikh <[email protected]>

ENI support

Signed-off-by: Sameer Shaikh <[email protected]>

ENI support

Signed-off-by: Sameer Shaikh <[email protected]>

ENI support

Signed-off-by: Sameer Shaikh <[email protected]>

ENI support

Signed-off-by: Sameer Shaikh <[email protected]>

ENI support

Signed-off-by: Sameer Shaikh <[email protected]>

ENI support

Signed-off-by: Sameer Shaikh <[email protected]>

ENI support

Signed-off-by: Sameer Shaikh <[email protected]>

PrimaryIP support

Signed-off-by: Sameer Shaikh <[email protected]>

Review comments

Signed-off-by: Sameer Shaikh <[email protected]>

Review comments

Signed-off-by: Sameer Shaikh <[email protected]>

Review comments

Signed-off-by: Sameer Shaikh <[email protected]>

Review comments

Signed-off-by: Sameer Shaikh <[email protected]>

Review comments

Signed-off-by: Sameer Shaikh <[email protected]>

Review comments

Signed-off-by: Sameer Shaikh <[email protected]>

Review comments

Signed-off-by: Sameer Shaikh <[email protected]>

Review comments

Signed-off-by: Sameer Shaikh <[email protected]>

Review Comments

Signed-off-by: Sameer Shaikh <[email protected]>

Review Comments

Signed-off-by: Sameer Shaikh <[email protected]>

Review Comments

Signed-off-by: Sameer Shaikh <[email protected]>

Review Comments

Signed-off-by: Sameer Shaikh <[email protected]>

Review Comments

Signed-off-by: Sameer Shaikh <[email protected]>

Review Comments

Signed-off-by: Sameer Shaikh <[email protected]>

Review Comments

Signed-off-by: Sameer Shaikh <[email protected]>

Review Comments

Signed-off-by: Sameer Shaikh <[email protected]>

Review Comments

Signed-off-by: Sameer Shaikh <[email protected]>

Review Comments

Signed-off-by: Sameer Shaikh <[email protected]>

Rebase master

Signed-off-by: Sameer Shaikh <[email protected]>

Review Comments

Signed-off-by: Sameer Shaikh <[email protected]>

Review Comments

Signed-off-by: Sameer Shaikh <[email protected]>

Adding support for GetSecurityGroupByName

Signed-off-by: Sameer Shaikh <[email protected]>

Adding support for GetSecurityGroupByName

Signed-off-by: Sameer Shaikh <[email protected]>

Adding support for GetSecurityGroupByName

Signed-off-by: Sameer Shaikh <[email protected]>

Adding support for GetSecurityGroupByName

Signed-off-by: Sameer Shaikh <[email protected]>

Adding support for GetSecurityGroupByName

Signed-off-by: Sameer Shaikh <[email protected]>

Adding support for GetSecurityGroupByName
Signed-off-by: Sameer Shaikh <[email protected]>
Signed-off-by: Sameer Shaikh <[email protected]>
Signed-off-by: Sameer Shaikh <[email protected]>
Signed-off-by: Sameer Shaikh <[email protected]>
Copy link

@skorati skorati left a comment

Choose a reason for hiding this comment

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

@sameshai Couple if comments on PR: https://github.com/IBM/ibmcloud-volume-file-vpc/pull/15/files

  1. Make the errorCodes homogeneous . Please use if SubnetFine* , SubnetList* as we are using Volume*Action* in other places.
  2. For ListSubnetsFailed and ListSecurityGroupsFailed, why are we returning 500, is this the only applicable error code.. is it possible that users will not have access or its forbidden action, can you confirm.
  3. Could you rephrase the below errors, they are bit ambiguous to me.
  • Description: "A subnet with the specified zone '%s' and available cluster subnet list '%s' could not be found.", -> instead of and available cluster , can we have in cluster subnet list

  • "A securityGroup with the specified cluster securityGroup name '%s' could not be found.", -> Its confusing if %s represents cluster name or security group name

  1. The names for these errorCodes are too long can we shorten/simplify them.
  • SecurityGroupFindFailedWithVPCAndSecurityGroupName
  • SubnetFindFailedWithZoneAndSubnetID

We can have webex if needed.

@sameshai
Copy link
Member Author

  1. Make the errorCodes homogeneous . Please use if SubnetFine* , SubnetList* as we are using Volume*Action* in other places.
    Done
  2. For ListSubnetsFailed and ListSecurityGroupsFailed, why are we returning 500, is this the only applicable error code.. is it possible that users will not have access or its forbidden action, can you confirm.

There is no API specific access errors if we have reached till this stage that means forbidden or permission issue is not ablicable. It will fail with actual error code by IAAS back end error. This is rare case like network issue in which we will say 500 error only

  1. Could you rephrase the below errors, they are bit ambiguous to me.
  • Description: "A subnet with the specified zone '%s' and available cluster subnet list '%s' could not be found.", -> instead of and available cluster , can we have in cluster subnet list*
  • "A securityGroup with the specified cluster securityGroup name '%s' could not be found.", -> Its confusing if %s represents cluster name or security group name

Done
4. The names for these errorCodes are too long can we shorten/simplify them.
SecurityGroupFindFailedWithVPCAndSecurityGroupName
SubnetFindFailedWithZoneAndSubnetID

Done

@skorati uploaded the changes as part of https://github.com/IBM/ibmcloud-volume-file-vpc/pull/25/files

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.

2 participants