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

Implement workaround for incompatible change in cloud foundry API #343

Merged
merged 9 commits into from
Oct 17, 2018
Merged

Implement workaround for incompatible change in cloud foundry API #343

merged 9 commits into from
Oct 17, 2018

Conversation

fwilhe
Copy link
Contributor

@fwilhe fwilhe commented Oct 15, 2018

changes:

Handle the case when cloud foundry manifest follows the new format (list of buildpacks) and transform to a single buildpack, since the blue-green plugin can't handle the new API.

  • add tests
  • add documentation

@CLAassistant
Copy link

CLAassistant commented Oct 15, 2018

CLA assistant check
All committers have signed the CLA.

}
application['buildpack'] = buildpacks[0]
}
application.remove('buildpacks')
Copy link
Member

Choose a reason for hiding this comment

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

What if buildpacks is not a List?

def handleLegacyCfManifest(config) {
def manifest = readYaml file: config.cloudFoundry.manifest
manifest = CfManifestUtils.transform(manifest)
sh "rm ${config.cloudFoundry.manifest}"
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we could skip writing the file, in case nothing has changed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As discussed, we prefer to keep it simple, and hence don't check for a change

Copy link
Member

@OliverNocon OliverNocon left a comment

Choose a reason for hiding this comment

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

From my point of view okay if we add some more info for the users.

cloudFoundryDeploy(
script: script,
deployType: 'blue-green',
cloudFoundry: target,
Copy link
Member

Choose a reason for hiding this comment

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

we might describe how target should look like ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure

def manifest = readYaml file: config.cloudFoundry.manifest
manifest = CfManifestUtils.transform(manifest)
sh "rm ${config.cloudFoundry.manifest}"
writeYaml file: config.cloudFoundry.manifest, data: manifest
Copy link
Member

Choose a reason for hiding this comment

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

we should add some information to the log that the manifest file has been re-written, maybe even with some references to cf documentation etc. to provide some background.

Without this we would silently do something which might be a problem for the users, especially in error situations

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My idea was to make it transparent in a way that the user does not have to know/care, and thus avoid noise. The whole thing is a bit complex to explain in one println, but a reference can be added, I agree.

sh "echo $originalManifest" //todo
sh "echo $transformedManifest" //todo
if (originalManifest != transformedManifest) {
echo "The file ${config.cloudFoundry.manifest} is not compatible with the Cloud Foundry blue-green deployment plugin. Re-writing inline."
Copy link
Member

Choose a reason for hiding this comment

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

please use a multiline string and one echo command

@fwilhe
Copy link
Contributor Author

fwilhe commented Oct 17, 2018

Thanks for your review @OliverNocon, I addressed all comments.

@@ -61,6 +61,10 @@ Deployment can be done
- cfOrg
- cfSpace

!!! note
Due to [an incompatible change](https://github.com/cloudfoundry/cli/issues/1445) in the Cloud Foundry CLI, multiple buildpacks are not supported by this step.
If your `application` contains a list of `buildpacks` instead a single `buildpack`, this will be automatically re-written by the step.
Copy link
Member

Choose a reason for hiding this comment

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

Only in case you are using the blue green deployment

@fwilhe fwilhe merged commit 7f7afda into SAP:master Oct 17, 2018
@fwilhe fwilhe deleted the fix/cf-manifest branch October 17, 2018 09:01
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