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

refactor: convert Jenkinsfile to declarative pipeline syntax #4784

Merged
merged 4 commits into from Jun 24, 2021

Conversation

@jdrueckert
Copy link
Member

@jdrueckert jdrueckert commented Jun 22, 2021

No description provided.

@jdrueckert
Copy link
Member Author

@jdrueckert jdrueckert commented Jun 22, 2021

Jenkinsfile Outdated
@@ -17,7 +18,7 @@ properties([
/**
* Main pipeline definition for building the engine.
*
* It uses the Scripted Pipeline Syntax.
* It uses the Declarative Pipeline Syntax.
* See https://www.jenkins.io/doc/book/pipeline/#declarative-versus-scripted-pipeline-syntax

This comment has been minimized.

@skaldarnar

skaldarnar Jun 22, 2021
Member

Since it uses declarative syntax now, I think we can now better link to https://www.jenkins.io/doc/book/pipeline/syntax/

@keturn
Copy link
Contributor

@keturn keturn commented Jun 24, 2021

This is clearly working. It's running the pipeline, the steps are all labelled, it's recording the results, etc.

but my head hurts because there were some things I assumed must be true about the Declarative Pipeline syntax, and this is refuting my assumptions.

For instance, I assumed that the way for Jenkins to support a different syntax was for that pipeline {} to be the only block, then it knows it's supposed to use the declarative parser for that file.

I assumed that the trick of converting these pipelines would be those specialBranch and artifactBuildsToKeep definitions.

But you just left them in there as-is? And it doesn't complain about it at all?

Sheesh.

@keturn
Copy link
Contributor

@keturn keturn commented Jun 24, 2021

So does "declarative pipeline syntax" actually use a different syntax at all, or are declarative Jenkinsfiles still evaluated as groovy and those are just a different set of methods, methods that are designed as a builder instead of doing immediate execution?

@jdrueckert
Copy link
Member Author

@jdrueckert jdrueckert commented Jun 24, 2021

So does "declarative pipeline syntax" actually use a different syntax at all, or are declarative Jenkinsfiles still evaluated as groovy and those are just a different set of methods, methods that are designed as a builder instead of doing immediate execution?

@keturn according to https://www.jenkins.io/doc/book/pipeline/#declarative-versus-scripted-pipeline-syntax they are rather similar in "many of the individual syntactical components", so I guess from the basic structure they look pretty similar. The key difference seems to be the pipeline (declarative) vs the node (scripted) blocks. As those are also the outermost parts, I guess that's where the parser get the info whether it's facing a scripted or declarative pipeline.

@keturn
keturn approved these changes Jun 24, 2021
Copy link
Contributor

@keturn keturn left a comment

I've tested to make sure that artifactBuildsToKeep setting is honored. Looks good so far and we should have more confirmation in another six minutes or so.

I haven't tested the Publish block (because PR branches don't get published), but I'm optimistic enough to go ahead and see how it works in practice.

engine/build/resources/main/org/terasology/engine/version/versionInfo.properties,
natives/**,
build-logic/src/**,
build-logic/*.kts

This comment has been minimized.

@keturn

keturn Jun 24, 2021
Contributor

+1 for this long-comma-separated-string formatting change

post {
always {
Comment on lines +70 to +71

This comment has been minimized.

@keturn

keturn Jun 24, 2021
Contributor

yep, replacing try/finally with post/always

-PmavenUser=${artifactoryUser}
-PmavenPass=${artifactoryPass}
'''
Comment on lines +96 to +98

This comment has been minimized.

@keturn

keturn Jun 24, 2021
Contributor

We will want to confirm that this doesn't leak the credentials into the easily-viewable logs, but I think this style of quote should be fine for that.

script {
// Trigger the Omega dist job to repackage a game zip with modules
Comment on lines +100 to +101

This comment has been minimized.

@keturn

keturn Jun 24, 2021
Contributor

Was not this inside the specialBranch conditional before? I agree it should be!

I'm not sure if there's a disadvantage to using a script block like this. I'm okay with assuming it's fine until we learn otherwise.

This comment has been minimized.

@jdrueckert

jdrueckert Jun 24, 2021
Author Member

The problem I faced here is that ther is this when clause to only run a stage if a specific condition is met, but from what I found this is only available for whole stages, not for individual steps.
The alternative would be to create four different stages, each with the matching when expression.
I figured that with the script block we avoid the duplication and additional stages shown in the ui (of which at max one will ever be executed in a pipeline run).

This comment has been minimized.

@jdrueckert

jdrueckert Jun 24, 2021
Author Member

And no, this was outside of the specialBranch conditional also before. Directly after it, to be precise.

@jdrueckert jdrueckert merged commit 5b9b6f7 into develop Jun 24, 2021
17 checks passed
17 checks passed
@github-actions
update_release_draft
Details
@lgtm-com
LGTM analysis: Java No code changes detected
Details
@GooeyHub
ci/cloudbees/checkstyle 87 issues
Details
@GooeyHub
ci/cloudbees/javadoc-warnings 159 issues
Details
@GooeyHub
ci/cloudbees/open-tasks 453 issues
Details
@GooeyHub
ci/cloudbees/pmd 456 issues
Details
@GooeyHub
ci/cloudbees/spotbugs 302 issues
Details
@GooeyHub
ci/cloudbees/stage/Analytics Finished
Details
@GooeyHub
ci/cloudbees/stage/Build Finished
Details
@GooeyHub
ci/cloudbees/stage/Declarative: Checkout SCM Finished
Details
@GooeyHub
ci/cloudbees/stage/Documentation Finished
Details
@GooeyHub
ci/cloudbees/stage/Integration Tests Finished
Details
@GooeyHub
ci/cloudbees/stage/Setup Finished
Details
@GooeyHub
ci/cloudbees/stage/Unit Tests Finished
Details
@GooeyHub
ci/cloudbees/tests/Integration Tests 10 tests were skipped (123 passed)
Details
@GooeyHub
ci/cloudbees/tests/Unit Tests 9 tests were skipped (638 passed)
Details
@GooeyHub
continuous-integration/jenkins/pr-merge This commit looks good
Details
@jdrueckert jdrueckert deleted the refactor/convert-pipeline-to-declarative branch Jun 24, 2021
@keturn
Copy link
Contributor

@keturn keturn commented Jun 24, 2021

Suggestions for future improvements:

  • Move discoverGitReferenceBuild as early as possible.
  • Publish stage: Refactor "rebuild Omega after engine trunk build" to not need script block
  • Analytics and Documentation stages: move recordIssues to post block
  • taskScanner: configure tags (haven't been using WIBNIF or ASAP)
  • add recordIssues tool: java (to Build stage?)

@jdrueckert
Copy link
Member Author

@jdrueckert jdrueckert commented Jun 24, 2021

Let's see in the master build that's being triggered now, whether the publish stage behaves properly.
If everything looks as expected, I'll migrate the module pipeline accordingly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants