Skip to content
This repository has been archived by the owner on Jan 5, 2022. It is now read-only.

Download the cx-server from github.com #9

Merged
merged 7 commits into from
Oct 23, 2019

Conversation

florian-richter
Copy link
Contributor

Proposed Changes

This changes the behavior of sap-cloud-sdk add-cx-server to get the cx-server and server.cfg from the master branch of the respective github repo instead of copying files locally.

This has the benefit that the files will always be the newest version, but it now requires internet connectivity to run (which can be assumed as the scripts need internet connectivity to run anyways).

Type of Changes

  • Bug Fix
  • New Feature
  • Documentation Update

Breaking Changes

In general avoid breaking changes, but if you think it is necessary give your reasoning in the Further Comments section.

  • I have not introduced breaking changes

Florian Richter added 2 commits October 22, 2019 14:35
# Conflicts:
#	src/commands/add-cx-server.ts
#	src/commands/init.ts
Copy link
Contributor

@mr-flannery mr-flannery left a comment

Choose a reason for hiding this comment

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

Rather questions than requests for changes.

}

function copyLocal(sourcePath: string, targetFolder: string, fileName: string, options: { [key: string]: any }) {
return new Promise((resolve, reject) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick: you could save one level of indentation across the whole function by returning Promise.resolve() or Promise.reject(e), respectively. Personally I find this easier to read.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you be more explicit? I tried to do it that way but I couldn't figure out how. But maybe I am missing something obvious...

src/commands/init.ts Outdated Show resolved Hide resolved
Comment on lines 33 to 44
const files = [
{
sourcePath: new URL('cx-server', githubPrefix),
targetFolder: path.resolve(flags.projectDir, 'cx-server'),
fileName: path.resolve(flags.projectDir, 'cx-server', 'cx-server')
},
{
sourcePath: new URL('server.cfg', githubPrefix),
targetFolder: path.resolve(flags.projectDir, 'cx-server'),
fileName: path.resolve(flags.projectDir, 'cx-server', 'server.cfg')
}
];
Copy link
Contributor

Choose a reason for hiding this comment

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

what happened to the .bat file? Shouldn't we keep Windows support?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also what happened to the concern about the number of relevant files potentially changing? No proper/sane way to implement it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So this is the result from a discussion with the pipeline team. They said that they consider only those two files essential and also consider the naming and folder structure as stable. So this seemed like the easiest way.

There are downsides, like the risk of breaking this command when the file names change. But I feel any other solution would likely be over-engineered or slow.

Henning also pointed out that we should keep the .bat while the pipeline team seemed to dismiss it. I will add it back in if the command is executed on a Windows machine.

@florian-richter florian-richter merged commit 3189da9 into master Oct 23, 2019
@florian-richter florian-richter deleted the download-cx-server branch October 23, 2019 07:57
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants