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 fix: integration tests are now built in the user namespace #412

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

WIP fix: integration tests are now built in the user namespace #412

wants to merge 2 commits into from

Conversation

bartoszmajsak
Copy link
Contributor

Due to misconfigured arquillian.xml and environment variables in the pipeline library integration tests are currently executed in the user-jenkins namespace which is not intended for that nor has enough resources - see openshiftio/openshift.io#3134 (comment)

The idea always was to use user namespace for that purpose. Therefore arquillian.xml is configured in a way that when executing tests in Che it will pick up current namespace (and it happens to be user).

This fix brings the same behaviour to the CI build.

Open points

Before we merge, let's discuss one thing, as the code in the current shape is not something I'm happy about.

Is there a way to set up some config.properties which we can than refer to in the pipeline library?

I know we have a hook .openshiftio/Jenkins.setup.groovy with setupEnvironmentPost(). I was wondering if we can externalized things like profiles and env variables we can pass for the build as mvn arguments, as right now have fixed in the pipeline itself (i.e. -P openshift - which not every project can have). This way we can move away some booster-specific settings for OSIO and do not pollute the code of the pipeline library. I see there is ${config.itestPattern} so was wondering if that concept can be somehow re-used

Fixes openshiftio/openshift.io#3134 openshiftio/openshift.io#763

@hrishin
Copy link
Member

hrishin commented Aug 2, 2018

@bartoszmajsak is it a good idea to keep it into Jenkinsfile only?

@bartoszmajsak
Copy link
Contributor Author

@hrishin can you expand on what you mean?

@@ -29,7 +29,7 @@ def call(body) {
echo "WARNING: Integration tests are current DISABLED for these pipelines!"

} else {
sh "mvn org.apache.maven.plugins:maven-failsafe-plugin:integration-test ${kubeNS} -P openshift-it -Dit.test=${config.itestPattern} -DfailIfNoTests=${config.failIfNoTests} org.apache.maven.plugins:maven-failsafe-plugin:verify"
sh "mvn org.apache.maven.plugins:maven-failsafe-plugin:integration-test ${kubeNS} -Dnamespace.use.current=false -DenableImageStreamDetection=true -P openshift-it -Dit.test=${config.itestPattern} -DfailIfNoTests=${config.failIfNoTests} org.apache.maven.plugins:maven-failsafe-plugin:verify"
Copy link
Contributor

Choose a reason for hiding this comment

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

How about adding a comment explaining all parameters we are sending, just to understand in some months why we set them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't want these parameters to be here to start with - see my "Open points" section in the PR description.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes I know but I put the comment just in case we decided for now to go with this approach.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we decide, I will :)

@bartoszmajsak
Copy link
Contributor Author

bartoszmajsak commented Aug 9, 2018

Based on the discussion today let's recap what we can do with that issue, as I'm not sure we reached the conclusion. Here are the options I can think of:

  • merge - keep it as ugly as it is (we are not making it uglier ;))
  • open it up for customization, where possible solution could be
    • rely on already passed config object and customize on how mavenCI{} and mavenCD{} are invoked (so replace this or this with something like mavenCI{build: '.....'} - requires further analysis of what we want to make customizable - first cut would be those places where we explictly define profiles (so testing and deployment)
      • as a consquence we have to adjust Jenkinsfiles which come with imported projects (through Launcher), as they will have to now define their way of building/testing
    • I think extending Jenkinsfile.setup.groovy which we use for Booster hooks should not be considered as the right path

I think this is an important point to address. My overall concern is that the library as is now, makes it hard for any project to customize, and we all know that even in the mvn world where there is a strong focus on conventions we very frequently break out of it using profiles, properties etc (as this PR demonstrated anyway).

@sthaha @hrishin @gorkem WDYT?

@hrishin
Copy link
Member

hrishin commented Aug 10, 2018

I think extending Jenkinsfile.setup.groovy which we use for Booster hooks should not be considered as the right path

Feels the same. It appears like the the purpose of the setup.groovy is to have pre or post build hooks and things we are referring here is about customizing one of the build steps. right?

At this moment this library is failing about one thing "speration of concern, isolating thing that is subject to vary from project to project like mvn command".
How about extract the maven command to projects Jenkinsfile? In that way user would know the build command that build going to execute for given project. This path would require us to change many things accroos consumer of this library, however in log run it could pay off.

Otherwise being pragmatic, I agree with @bartoszmajsak approach to provide customization for mavenCI{}, mavenCD{} and other clsures to accept profiles, builds flags and probably goals (build targets) as well. This closure config would get more precedence over default one.

@hrishin
Copy link
Member

hrishin commented Aug 10, 2018

@bartoszmajsak voices heard. #418

Can you we try to follow https://github.com/fabric8io/fabric8-pipeline-library#maven-canary-release.? The same options are avaliable now with mavenCI{}.

@bartoszmajsak
Copy link
Contributor Author

Thanks @hrishin.

In order to make this fix working we also need to pass this cmd here

mavenIntegrationTest {
environment = 'Test'
failIfNoTests = false
itestPattern = '*IT'
}

and adjust this line

sh "mvn org.apache.maven.plugins:maven-failsafe-plugin:integration-test ${kubeNS} -P openshift-it -Dit.test=${config.itestPattern} -DfailIfNoTests=${config.failIfNoTests} org.apache.maven.plugins:maven-failsafe-plugin:verify"

hrishin added a commit to hrishin/fabric8-pipeline-library that referenced this pull request Aug 10, 2018
This patch allows the  to over maven
command in order to allow inetgration tests to pass specific
maven flags.

Related to: fabric8io#412
hrishin added a commit to hrishin/fabric8-pipeline-library that referenced this pull request Aug 10, 2018
This patch allows the  to over maven command in order
to allow integration tests to pass specific maven flags.

Related to: fabric8io#412
hrishin added a commit to hrishin/fabric8-pipeline-library that referenced this pull request Aug 10, 2018
This patch allows the  to over maven command in order
to allow integration tests to pass specific maven flags.

Related to: fabric8io#412
hrishin added a commit to hrishin/fabric8-pipeline-library that referenced this pull request Aug 13, 2018
This patch allows the `mavenCI{}` to over maven command in order
to allow `mavenIntegrationTes{}` to pass specific command to execute.

Fixes: openshiftio/openshift.io#763
Fixes: openshiftio/openshift.io#3134
Related to: fabric8io#412
hrishin added a commit to hrishin/fabric8-pipeline-library that referenced this pull request Aug 13, 2018
This patch allows the `mavenCI{}` to over maven command in order
to allow `mavenIntegrationTes{}` to pass specific command to execute.

Fixes: openshiftio/openshift.io#763
Fixes: openshiftio/openshift.io#3134
Related to: fabric8io#412
hrishin added a commit to hrishin/fabric8-pipeline-library that referenced this pull request Aug 13, 2018
This patch allows the `mavenCI{}` to over maven command in order
to allow `mavenIntegrationTes{}` to pass specific command to execute.

Fixes: openshiftio/openshift.io#763
Fixes: openshiftio/openshift.io#3134
Related to: fabric8io#412
hrishin added a commit to hrishin/fabric8-pipeline-library that referenced this pull request Aug 14, 2018
This patch allows the `mavenCI{}` to over maven command in order
to allow `mavenIntegrationTes{}` to pass specific command to execute.

Fixes: openshiftio/openshift.io#763
Fixes: openshiftio/openshift.io#3134
Related to: fabric8io#412
hrishin added a commit to hrishin/fabric8-pipeline-library that referenced this pull request Aug 14, 2018
This patch allows the `mavenCI{}` to over maven command in order
to allow `mavenIntegrationTes{}` to pass specific command to execute.
This patch adds `runTestsInUserNamespace` to use username namespace as defualt default environment to run
Integration Tests.

Fixes: openshiftio/openshift.io#763
Fixes: openshiftio/openshift.io#3134
Related to: fabric8io#412
hrishin added a commit to hrishin/fabric8-pipeline-library that referenced this pull request Aug 14, 2018
This patch allows the `mavenCI{}` to over maven command in order
to allow `mavenIntegrationTes{}` to pass specific command to execute.
This patch adds `runTestsInUserNamespace` to use username namespace as defualt default environment to run
Integration Tests.

Fixes: openshiftio/openshift.io#763
Fixes: openshiftio/openshift.io#3134
Related to: fabric8io#412
hrishin added a commit to hrishin/fabric8-pipeline-library that referenced this pull request Aug 14, 2018
This patch allows the `mavenCI{}` to over maven command in order
to allow `mavenIntegrationTes{}` to pass specific command to execute.
This patch adds `runTestsInUserNamespace` to use username namespace as defualt default environment to run
Integration Tests.

Fixes: openshiftio/openshift.io#763
Fixes: openshiftio/openshift.io#3134
Related to: fabric8io#412
hrishin added a commit to hrishin/fabric8-pipeline-library that referenced this pull request Aug 14, 2018
This patch allows the `mavenCI{}` to over maven command in order
to allow `mavenIntegrationTes{}` to pass specific command to execute.
This patch adds `runTestsInUserNamespace` to use username namespace as defualt default environment to run
Integration Tests.

Fixes: openshiftio/openshift.io#763
Fixes: openshiftio/openshift.io#3134
Related to: fabric8io#412
hrishin added a commit to hrishin/fabric8-pipeline-library that referenced this pull request Aug 14, 2018
This patch allows the `mavenCI{}` to over maven command in order
to allow `mavenIntegrationTes{}` to pass specific command to execute.
This patch adds `runTestsInUserNamespace` to use username namespace as defualt default environment to run
Integration Tests.

Fixes: openshiftio/openshift.io#763
Fixes: openshiftio/openshift.io#3134
Related to: fabric8io#412
hrishin added a commit to hrishin/fabric8-pipeline-library that referenced this pull request Aug 14, 2018
This patch allows the `mavenCI{}` to over maven command in order
to allow `mavenIntegrationTes{}` to pass specific command to execute.
This patch adds `runTestsInUserNamespace` to use username namespace as defualt default environment to run
Integration Tests.

Fixes: openshiftio/openshift.io#763
Fixes: openshiftio/openshift.io#3134
Related to: fabric8io#412
hrishin added a commit to hrishin/fabric8-pipeline-library that referenced this pull request Aug 14, 2018
This patch allows the `mavenCI{}` to over maven command in order
to allow `mavenIntegrationTes{}` to pass specific command to execute.
This patch adds `runTestsInUserNamespace` to use username namespace as defualt default environment to run
Integration Tests.

Fixes: openshiftio/openshift.io#763
Fixes: openshiftio/openshift.io#3134
Related to: fabric8io#412
hrishin added a commit to hrishin/fabric8-pipeline-library that referenced this pull request Aug 14, 2018
This patch allows the `mavenCI{}` to over maven command in order
to allow `mavenIntegrationTes{}` to pass specific command to execute.
This patch adds `runTestsInUserNamespace` to use username namespace as defualt default environment to run
Integration Tests.

Fixes: openshiftio/openshift.io#763
Fixes: openshiftio/openshift.io#3134
Related to: fabric8io#412
hrishin added a commit to hrishin/fabric8-pipeline-library that referenced this pull request Aug 16, 2018
This patch allows the `mavenCI{}` to over maven command in order
to allow `mavenIntegrationTes{}` to pass specific command to execute.
It has added a util method to retrive default test namespace.

Fixes: openshiftio/openshift.io#763
Fixes: openshiftio/openshift.io#3134
Related to: fabric8io#412
hrishin added a commit to hrishin/fabric8-pipeline-library that referenced this pull request Aug 16, 2018
This patch allows the `mavenCI{}` to over maven command in order
to allow `mavenIntegrationTes{}` to pass specific command to execute.
It has added a util method to retrive default test namespace.

Fixes: openshiftio/openshift.io#763
Fixes: openshiftio/openshift.io#3134
Related to: fabric8io#412
hrishin added a commit to hrishin/fabric8-pipeline-library that referenced this pull request Aug 16, 2018
This patch allows the `mavenCI{}` to over maven command in order
to allow `mavenIntegrationTes{}` to pass specific command to execute.
It has added a util method to retrive default test namespace.

Fixes: openshiftio/openshift.io#763
Fixes: openshiftio/openshift.io#3134
Related to: fabric8io#412
sthaha pushed a commit that referenced this pull request Aug 24, 2018
* mavenIntegrationTest: allow overriding mvn commands

This patch allows the `mavenCI{}` to over maven command in order
to allow `mavenIntegrationTest{}` to pass specific command to execute.
It has added a util method to retrieve default test namespace.

Fixes: openshiftio/openshift.io#763
Fixes: openshiftio/openshift.io#3134
Related to: #412

* refactored code as per review comments
- extracted default maven command code to separate method
- changed variable names
- updated readme
@hrishin
Copy link
Member

hrishin commented Aug 24, 2018

@bartoszmajsak shall we close this PR?

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.

ConfigMap / Wildfly quickstart fails to build
3 participants