Test and implementation

[danielfoglio]

closes cratekube/cratekube#62

Signed-off-by: Daniel Foglio dfoglio@cisco.com

---

This change is 

[danielfoglio]

+@kod4n +@code4dc

Reviewable status: 0 of 2 LGTMS obtained (waiting on @code4dc and @kod4n)

[kod4n]

Reviewed 13 of 15 files at r1.
Reviewable status: 0 of 2 LGTMS obtained (waiting on @code4dc and @kod4n)

---

src/main/groovy/io/cratekube/operations/resources/OperationsResource.groovy, line 61 at r1 (raw file):

    @ApiParam(hidden = true) @Auth User user
  ) {
    require body?.name, notEmptyString()

If you want to enforce not empty on the post body I would recommend adding to appropriate annotation to the EnvironmentClusterRequest class property and then adding @Valid to the body parameter above. That will make sure requests are blocked that have an empty or null name.

[kod4n]

Reviewed 1 of 15 files at r1.
Reviewable status: 0 of 2 LGTMS obtained (waiting on @code4dc and @kod4n)

---

src/main/groovy/io/cratekube/operations/service/OperationsService.groovy, line 49 at r1 (raw file):

      log.debug 'Creating infrastructure for [{}]', name
      cloudMgmtApi.createEnvironment(null, new EnvironmentRequest(name: name)).execute()

are any of these calls to the cloudMgmtApi or clusterMgmtApi going to work if we are not passing the bearer token?

[kod4n]

Reviewed 1 of 15 files at r1.
Reviewable status: 0 of 2 LGTMS obtained (waiting on @code4dc and @kod4n)

---

src/main/groovy/io/cratekube/operations/service/OperationsService.groovy, line 41 at r1 (raw file):

    require name, notEmptyString()

    executor.execute {

Can we run into an issue here if multiple requests are triggered for the same cluster name in rapid succession?

[danielfoglio]

Reviewable status: 0 of 2 LGTMS obtained (waiting on @code4dc and @kod4n)

---

src/main/groovy/io/cratekube/operations/resources/OperationsResource.groovy, line 61 at r1 (raw file):

Previously, kod4n (Andrew Allen) wrote…

If you want to enforce not empty on the post body I would recommend adding to appropriate annotation to the EnvironmentClusterRequest class property and then adding @Valid to the body parameter above. That will make sure requests are blocked that have an empty or null name.

Ohhhh yeah. I will get that taken care of. Good call.

---

src/main/groovy/io/cratekube/operations/service/OperationsService.groovy, line 41 at r1 (raw file):

Previously, kod4n (Andrew Allen) wrote…

Can we run into an issue here if multiple requests are triggered for the same cluster name in rapid succession?

Hmm it is possible...

As soon as that create is called on the cloud-mgmt-service a record is placed in its cache and subsequent requests will return an environment with a pending status.

I think if we move the environmentClusters check and the createEnvironment call to outside the executor that would be enough. Thoughts?

---

src/main/groovy/io/cratekube/operations/service/OperationsService.groovy, line 49 at r1 (raw file):

Previously, kod4n (Andrew Allen) wrote…

are any of these calls to the cloudMgmtApi or clusterMgmtApi going to work if we are not passing the bearer token?

I have the bearer token configured in the modules as an authorization on the request chain

[danielfoglio]

Reviewable status: 0 of 2 LGTMS obtained (waiting on @code4dc and @kod4n)

---

src/main/groovy/io/cratekube/operations/service/OperationsService.groovy, line 49 at r1 (raw file):

Previously, danielfoglio (Daniel Foglio) wrote…

I have the bearer token configured in the modules as an authorization on the request chain

It was done in a prior pr I don't think you saw.

[kod4n]

Reviewable status: 0 of 2 LGTMS obtained (waiting on @code4dc and @kod4n)

---

src/main/groovy/io/cratekube/operations/service/OperationsService.groovy, line 49 at r1 (raw file):

Previously, danielfoglio (Daniel Foglio) wrote…

It was done in a prior pr I don't think you saw.

In that case, do we need to use an implicit param for the authorization header? Seems like we have an extra parameter on every call that we don't really need.

[danielfoglio]

Reviewable status: 0 of 2 LGTMS obtained (waiting on @code4dc and @kod4n)

---

src/main/groovy/io/cratekube/operations/service/OperationsService.groovy, line 49 at r1 (raw file):

Previously, kod4n (Andrew Allen) wrote…

In that case, do we need to use an implicit param for the authorization header? Seems like we have an extra parameter on every call that we don't really need.

I think so. I can set up a PR for the cloud-mgmt-service.

[danielfoglio]

Reviewable status: 0 of 2 LGTMS obtained (waiting on @code4dc and @kod4n)

---

src/main/groovy/io/cratekube/operations/service/OperationsService.groovy, line 49 at r1 (raw file):

Previously, danielfoglio (Daniel Foglio) wrote…

I think so. I can set up a PR for the cloud-mgmt-service.

Done.

[kod4n]

Reviewed 3 of 3 files at r2.
Reviewable status: 0 of 2 LGTMS obtained (waiting on @code4dc and @kod4n)

---

src/main/groovy/io/cratekube/operations/service/OperationsService.groovy, line 41 at r1 (raw file):

Previously, danielfoglio (Daniel Foglio) wrote…

Hmm it is possible...

As soon as that create is called on the cloud-mgmt-service a record is placed in its cache and subsequent requests will return an environment with a pending status.

I think if we move the environmentClusters check and the createEnvironment call to outside the executor that would be enough. Thoughts?

sounds good

[kod4n]

I'm curious why we need a cron job for this service, but

Reviewable status: 1 of 2 LGTMS obtained (waiting on @code4dc)

[danielfoglio]

@kod4n We initially didn't think we were going to automatically provision a default environment if one does not exist. I asked Phil and he said that we want a default cluster created at application start if it does not exist. I wanted to have it delayed a little bit to give the other services time to start when they were all deployed by the lifecycle-serivce. My solution was a Job/

Reviewable status: 1 of 2 LGTMS obtained (waiting on @code4dc)

[code4dc]

with very minor comments

Reviewable status: complete! 2 of 2 LGTMS obtained

---

src/main/groovy/io/cratekube/operations/service/OperationsService.groovy, line 69 at r2 (raw file):

  Map<String, EnvironmentCluster> getEnvironmentClusters() {
    def environments = cloudMgmtApi.environments.execute().body()
    if (!environments) {

Can we do this as a one liner with a ternary operator? (nitpicking)

---

src/main/groovy/io/cratekube/operations/service/OperationsService.groovy, line 142 at r2 (raw file):

    Environment environment = null
    def attempts = 0
    while (environment?.status != status && attempts < TEN) {

Should we change the method to pass it the # of attempts or you don't think that will be useful?

---

src/main/groovy/io/cratekube/operations/service/OperationsService.groovy, line 166 at r2 (raw file):

      log.debug 'Waiting for cluster [{}] to reach status [{}]. Attempt [{}] of 10. Current status [{}].', name, status, attempts, cluster?.nodes?.first()?.status
      if (attempts > 0) {
        sleep THIRTY_THOUSAND

suggestion to change it to THIRTY_THOUSAND_MILLIS or THIRTY_SECS_IN_MILLIS just so its clear

[kod4n]

It seems like the bootstrapping phase would just invoke this API, maybe we could modify the job in the future so that it can be enabled/disabled via an env variable.

Reviewable status: complete! 2 of 2 LGTMS obtained