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

[WIP] AEROGEAR-1924 CLI service integration #51

Closed

Conversation

dimitraz
Copy link
Contributor

Describe what this PR does and why we need it:
Review existing command for integrating mobile services

Changes proposed in this pull request

  • Add tests for integration commands
  • Add a watch on the binding resource and report on its status
  • Add a --no-wait to exit immediately
  • Review documentation

To do

  • Need to properly test the integration and watch command, currently waiting on this issue

Which issue this PR fixes (This will close that issue when PR gets merged)
AEROGEAR-1924

}

// exit immediately
noWait, err := cmd.PersistentFlags().GetBool("no-wait")
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the logic here should go:

if noWait && ! redeploy{
  return 
}
//wait logic

if redeploy{
//redeploy logic
}

If you want me to assist here I am happy to do so

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What happens if a user specifies no-wait and redeploy? I'm guessing that's not possible so I could just send back the please redeploy message?

Copy link
Contributor

Choose a reason for hiding this comment

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

As mentioned on IRC we have to wait if auto redeploy is set. We will document this in the command docs

@@ -107,12 +108,12 @@ func (bc *IntegrationCmd) CreateIntegrationCmd() *cobra.Command {
cmd := &cobra.Command{
Use: "integration <consuming_service_instance_id> <providing_service_instance_id>",
Short: "integrate certain mobile services together",
Long: `example usage: kubectl plugin mobile create integration <consuming_service_instance_id> <providing_service_instance_id>
Copy link
Contributor

Choose a reason for hiding this comment

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

I think in the long description we should give an example of how to get the serviceinstance ids

mobile get serviceinstances fh-sync-server
mobile get serviceinstances keycloak

}
}

func TestIntegrationCmd_GetIntegrationCmd(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can remove this unless you are intending to fill it in

Copy link
Contributor

@maleck13 maleck13 left a comment

Choose a reason for hiding this comment

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

small changes but looking pretty good overall. 💯

@maleck13
Copy link
Contributor

@dimitraz can you rebase pls

@dimitraz dimitraz force-pushed the AEROGEAR-1924-service-binding branch from e8a18a1 to e1d5902 Compare January 31, 2018 12:15
@dimitraz dimitraz force-pushed the AEROGEAR-1924-service-binding branch from e1d5902 to 3c0710e Compare January 31, 2018 12:16
@dimitraz
Copy link
Contributor Author

Closing, see #58

@dimitraz dimitraz closed this Jan 31, 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.

2 participants