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

ci: Update workflow file #1639

Merged
merged 4 commits into from
Mar 2, 2021
Merged

ci: Update workflow file #1639

merged 4 commits into from
Mar 2, 2021

Conversation

pawelpasterz
Copy link
Contributor

@pawelpasterz pawelpasterz commented Mar 1, 2021 •
edited
Loading

PR adds missing IT workflow logic

Test Plan

How do we know the code works?

  1. run ./gradlew build
  2. run 
    export IT_STATE='{
  "windows-latest": "success",
  "windows-latest-bs": "any/url/windows",
  "ubuntu-latest": "SUCCESS",
  "ubuntu-latest-bs": "any/url/ununtu",
  "macos-latest": "success",
  "macos-latest-bs": "any/url/mac"
}'
  3. (?) optional -- you can change flank scripts settings (for example repo address) incommon/flank-debug.properties
  4. run 
    java -jar ./flank-scripts/build/libs/flank-scripts.jar integration_tests process_results \
    --global-result=failure \
    --rr=$IT_STATE  \
    --github-token=[your gh token] \
    --run-id=123456
  5. A new issue is created, an example from my fork Full Suite integration tests failed on master pawelpasterz/flank#123

Full workflow can be tested once merged only

@pawelpasterz pawelpasterz self-assigned this Mar 1, 2021
@pawelpasterz
Copy link
Contributor Author

@flank-it

@github-actions
Copy link
Contributor

github-actions bot commented Mar 1, 2021 •
edited
Loading

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@github-actions
Copy link
Contributor

github-actions bot commented Mar 1, 2021 •
edited
Loading

Integration tests succeed for all OSes ✅
Windows Build scan:
MacOS Build scan: https://gradle.com/s/vd4625rdrmwpy
Linux Build scan:
Workflow run https://github.com/Flank/flank/actions/runs/609927270

@github-actions
Copy link
Contributor

github-actions bot commented Mar 1, 2021 •
edited
Loading

Timestamp: 2021-03-02 06:35:05
Buildscan url for ubuntu-workflow run 613097104

@pawelpasterz pawelpasterz marked this pull request as ready for review March 1, 2021 12:35
Sloox
Sloox suggested changes Mar 1, 2021
View reviewed changes
Copy link
Contributor

@Sloox Sloox left a comment

Choose a reason for hiding this comment

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

My goodness i made some bad spelling mistakes here :(

.github/workflows/full_suite_integration_tests.yml Outdated
--result=${{ needs.run-it-full-suite.outputs.job_status }} \
--url=${{ needs.run-it-full-suite.outputs.build-scan-url }} \
--global-result=${{ needs.run-it-full-suite.outputs.job_status }} \
--run-result=${{ steps.state-json.outputs.run-state }} \
Copy link
Contributor

Choose a reason for hiding this comment

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

There shouldnt be a need to set a whole new var? Simply using the env variable will give what you are expecting and the

run: |
          STATE_JSON='{
            "windows-latest": "${{ env.windows-latest }}",
            "windows-latest-bs": "${{ env.windows-latest-bs }}",
            "ubuntu-latest": "${{ env.ubuntu-latest }}",
            "ubuntu-latest-bs": "${{ env.ubuntu-latest-bs }}",
            "macos-latest": "${{ env.macos-latest }}",
            "macos-latest-bs": "${{ env.macos-latest-bs }}"
          }'
          echo "::set-output name=run-state::$STATE_JSON"

is no longer needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Sloox Oh, that's neat! Thanks!
so it will be --run-result=${{ env }} ?

Copy link
Contributor

Choose a reason for hiding this comment

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

env specifically is the JSON string i was modelling it over.

Copy link
Contributor

Choose a reason for hiding this comment

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

@pawelpasterz yes

...cripts/src/main/kotlin/flank/scripts/ops/integrationtests/common/IntegrationResultContext.kt
val macOsBSUrl: String = "",
@SerialName("ubuntu-latest")
val linuxResult: ITResult = ITResult.FAILURE,
@SerialName("ubuntut-latest-bs")
Copy link
Contributor

Choose a reason for hiding this comment

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

my goodness what a bad spelling mistake 😲

flank-scripts/src/main/kotlin/flank/scripts/ops/integrationtests/common/ITResult.kt
enum class ITResult {
SUCCESS, FAILURE
}

object ITResultSerializer : KSerializer<ITResult> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we sure this is needed?
@SerialName should do the trick?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would, my intention was that scripts would handle both SUCCESS and success, not only lower cases

Copy link
Contributor

Choose a reason for hiding this comment

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

I am shocked they have yet to implement a type of alias as apart of the @SerialName parameter.
Kotlin/kotlinx.serialization#203
is the only info they have

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, and I can see that related PR is stuck :/

@pawelpasterz pawelpasterz requested a review from Sloox March 1, 2021 14:29
piotradamczyk5
piotradamczyk5 suggested changes Mar 1, 2021
View reviewed changes
Copy link
Contributor

@piotradamczyk5 piotradamczyk5 left a comment

Choose a reason for hiding this comment

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

waiting for release to be merged

@piotradamczyk5 piotradamczyk5 self-requested a review March 1, 2021 19:43
@pawelpasterz pawelpasterz enabled auto-merge (squash) March 2, 2021 06:38
@pawelpasterz pawelpasterz merged commit bb1741d into master Mar 2, 2021
@pawelpasterz pawelpasterz deleted the 1635-follow-up branch March 2, 2021 06:39
@github-actions github-actions bot locked and limited conversation to collaborators Mar 2, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Sloox Sloox Sloox approved these changes

@piotradamczyk5 piotradamczyk5 piotradamczyk5 approved these changes

@adamfilipow92 adamfilipow92 Awaiting requested review from adamfilipow92

@zuziaka zuziaka Awaiting requested review from zuziaka

@jan-goral jan-goral Awaiting requested review from jan-goral

Assignees

@pawelpasterz pawelpasterz

Labels
Snake 🐍
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@pawelpasterz @Sloox @piotradamczyk5