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.

Sign up for GitHub

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

Jump to bottom

Validate neo home inside neoDeploy #93

Merged
merged 4 commits into from
Mar 6, 2018
Merged

Validate neo home inside neoDeploy #93

merged 4 commits into from
Mar 6, 2018

Conversation

marcusholl
Copy link
Member

No description provided.

@marcusholl marcusholl force-pushed the pr/neoHomeValidationInsideNeoDeploy branch 2 times, most recently from 151aeff to 807bc5d Compare February 20, 2018 14:37
marcusholl
marcusholl commented Feb 20, 2018
View reviewed changes
vars/neoDeploy.groovy Outdated
@@ -87,6 +87,21 @@ def call(parameters = [:]) {
stepConfiguration, stepConfigurationKeys,
ConfigurationLoader.defaultStepConfiguration(script, stepName))

HOME_CHECK: {
Copy link
Member Author

Choose a reason for hiding this comment

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

  • The behaviour with validation inside the ui5 reference pipeline was:
    • NEO_HOME needs to be provided from the config. If it was not provided, we failed. The code with launching neo.sh from the path was not used.
  • The behaviour now is:
    • In case NEO_HOME is provided (either from config or as Jenkins environment variable), we validate. In case it is not provided we try to launch neo.sh from the path. This is the behaviour we had already when using the step outside our ui5 pipeline.

Copy link
Contributor

Choose a reason for hiding this comment

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

But if NEO_HOME is in the path the neo version must be validated anyways.

Copy link
Member Author

@marcusholl marcusholl Feb 23, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Agreed, but currently this is not possible with the current version of toolValidate. --> Should be done in another PR.

The current story is about moving the validation from the pipeline into the steps. Before there was also no validation in case neo.sh was fetched from the path. From that point of view this is no change wrt status before. But in general I aggree, we should also validate the version if we have neo in the path. But this should be added in another PR closer to the validation toolset.

Some more background: using which we get the full path to the executable. toolValidate works upon NEO_HOME which is not the path to the executable. In case which neo.sh returns a symbolic link (typically provided in /usr/binor /usr/local/bin) it is not even possible to strip the trailing tools/neo.sh.

@marcusholl
Copy link
Member Author

marcusholl commented Feb 20, 2018 •
edited
Loading

Integration test with pipeline not performing validation at the beginning has been performed successfully.

@marcusholl marcusholl force-pushed the pr/neoHomeValidationInsideNeoDeploy branch 5 times, most recently from a39fb19 to 9dcdde0 Compare February 22, 2018 07:53
@marcusholl marcusholl force-pushed the pr/neoHomeValidationInsideNeoDeploy branch from 9dcdde0 to 5fe99ce Compare February 22, 2018 09:10
alejandraferreirovidal
alejandraferreirovidal previously requested changes Feb 22, 2018
View reviewed changes
vars/neoDeploy.groovy Outdated
@@ -87,6 +87,21 @@ def call(parameters = [:]) {
stepConfiguration, stepConfigurationKeys,
ConfigurationLoader.defaultStepConfiguration(script, stepName))

HOME_CHECK: {
Copy link
Contributor

Choose a reason for hiding this comment

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

But if NEO_HOME is in the path the neo version must be validated anyways.

test/groovy/NeoDeploymentTest.groovy Outdated
// toolValidate here. This is expected to be done in a test class for
// toolValidate.
//
helper.registerAllowedMethod('toolValidate', [Map], {m -> toolValidateCalled = true;})
Copy link
Contributor

Choose a reason for hiding this comment

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

typo ;

Copy link
Member Author

Choose a reason for hiding this comment

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

I cannot see a typo, please help me with finding the typo.

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean using ; with Groovy

@marcusholl marcusholl force-pushed the pr/neoHomeValidationInsideNeoDeploy branch 2 times, most recently from f6a3840 to d148f82 Compare February 23, 2018 07:26
thorstenwillenbacher
thorstenwillenbacher reviewed Feb 23, 2018
View reviewed changes
vars/neoDeploy.groovy Outdated
@@ -196,7 +216,7 @@ def call(parameters = [:]) {

private getNeoExecutable(configuration) {

def neoExecutable = 'neo.sh' // default, if nothing below applies maybe it is the path.
def neoExecutable = NEO_DEFAULT_CMD // default, if nothing below applies maybe it is the path.

if (configuration.neoHome) {
neoExecutable = "${configuration.neoHome}/tools/neo.sh"
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe this occurance of neo.sh should also use NEO_DEFAULT_CMD

Copy link
Member Author

Choose a reason for hiding this comment

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

done.

@marcusholl marcusholl mentioned this pull request Feb 26, 2018
Merged
@marcusholl marcusholl force-pushed the pr/neoHomeValidationInsideNeoDeploy branch 3 times, most recently from ebda826 to 0dbb8be Compare February 26, 2018 15:41
@marcusholl marcusholl mentioned this pull request Feb 28, 2018
Merged
alxsap
alxsap reviewed Mar 1, 2018
View reviewed changes
vars/neoDeploy.groovy
// toolValidate.

def rc = sh script: "which ${NEO_DEFAULT_CMD}", returnStatus: true
if(neoHome || (!neoHome && rc != 0)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would directly fail here if (!neoHome && rc != 0) instead of passing an empty neohome ot toolValidate. This would be better readable/understandable.

Copy link
Contributor

Choose a reason for hiding this comment

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

Furthermore some log output could be helpful if neo is taken from the path.

Copy link
Member Author

Choose a reason for hiding this comment

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

We can discuss if we should fail here in case !neoHome. With the current approach the behavior is closer to the behavior before the change. Before the change we got inside toolValidateand fail inside in the same was like it is now.

Copy link
Member Author

Choose a reason for hiding this comment

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

log output: done

alejandraferreirovidal
alejandraferreirovidal reviewed Mar 5, 2018
View reviewed changes
Copy link
Contributor

@alejandraferreirovidal alejandraferreirovidal left a comment

Choose a reason for hiding this comment

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

my review is outdated

@marcusholl marcusholl force-pushed the pr/neoHomeValidationInsideNeoDeploy branch from 0dbb8be to 489dab7 Compare March 6, 2018 12:09
@marcusholl
remove docu generation
6d2dcd5 
since pip install fails caused by outdated python version
@marcusholl marcusholl force-pushed the pr/neoHomeValidationInsideNeoDeploy branch from 489dab7 to c6f4ce4 Compare March 6, 2018 15:14
@marcusholl marcusholl merged commit c6f4ce4 into SAP:master Mar 6, 2018
@marcusholl marcusholl deleted the pr/neoHomeValidationInsideNeoDeploy branch May 11, 2018 12:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thorstenwillenbacher thorstenwillenbacher thorstenwillenbacher left review comments

@alejandraferreirovidal alejandraferreirovidal alejandraferreirovidal left review comments

@alxsap alxsap alxsap approved these changes

@OliverNocon OliverNocon Awaiting requested review from OliverNocon

@CCFenner CCFenner Awaiting requested review from CCFenner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@marcusholl @thorstenwillenbacher @alejandraferreirovidal @alxsap