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

refactor: Fetching artifacts #1875

Merged
merged 8 commits into from
May 6, 2021
Merged

refactor: Fetching artifacts #1875

merged 8 commits into from
May 6, 2021

Conversation

pawelpasterz
Copy link
Contributor

Fixes #1758

Test Plan

How do we know the code works?

@github-actions
Copy link
Contributor

github-actions bot commented Apr 26, 2021 •
edited
Loading

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

pawelpasterz
pawelpasterz commented Apr 26, 2021
View reviewed changes
test_runner/src/main/kotlin/ftl/api/Artifacts.kt
val regex: List<Regex>,
val blobPath: String, // todo not needed?
val downloadPath: DownloadPath,
val matrixId: String
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added matrixId to persists info about matrix from which files are downloaded. It is used to update download flag once all files are fetched. Let me know if this violates any assumptions.
https://github.com/Flank/flank/pull/1875/files#diff-f91686b92f886f88f9b047c859da24c40bc6ec3ee8686ef5eec4bd5259f91de2R45

@jan-gogo

test_runner/src/main/kotlin/ftl/json/SavedMatrix.kt
val downloaded: Boolean = false,
// this is variable intentionally, this is workaround to pass changes while fetching artifacts
// todo think about different solution
var downloaded: Boolean = false,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to var. The old implementation had a bug and none of SavedMatrix had downloaded updated during artifact fetch. This is a quick fix for this issue. We should think about a different solution but in a separate issue.

Might be fixed within another refactor task (?)

@jan-gogo

@pawelpasterz pawelpasterz force-pushed the 1758-refactor-fetch-artifacts branch from 2423bf2 to e595475 Compare April 26, 2021 15:40
Sloox
Sloox reviewed Apr 27, 2021
View reviewed changes
test_runner/src/main/kotlin/ftl/adapter/google/FetchArtifacts.kt
val matrixName = parsed.getName(1).toString()
val fileName = parsed.fileName.toString()
val deviceName = parsed.getName(2).toString().takeUnless { it == fileName }.orEmpty()
val filePathName = if (downloadPath.keepFilePath) parsed.parent.drop(3).joinToString("/") else ""
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think its worth it to make these int variables constants with names for a little more clarity?

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 think I could change it to extension functions with self-explanatory names, WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

Those ints are explained by the value names so adding constants for them will be an unneeded redundancy. IMO this implementation looks ok.

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 fine with it as it is too :)

@pawelpasterz pawelpasterz force-pushed the 1758-refactor-fetch-artifacts branch from 046204f to 4e34968 Compare April 27, 2021 14:51
@github-actions
Copy link
Contributor

github-actions bot commented Apr 28, 2021 •
edited
Loading

Timestamp: 2021-05-06 15:31:44
Buildscan url for ubuntu-workflow run 817347855
https://gradle.com/s/xcdtybxd4galk

@pawelpasterz pawelpasterz force-pushed the 1758-refactor-fetch-artifacts branch 2 times, most recently from 405a907 to a900029 Compare April 28, 2021 16:12
@pawelpasterz
Copy link
Contributor Author

@flank-it

@github-actions
Copy link
Contributor

github-actions bot commented Apr 28, 2021 •
edited
Loading

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

@pawelpasterz pawelpasterz marked this pull request as ready for review April 28, 2021 17:09
@github-actions
Copy link
Contributor

github-actions bot commented Apr 28, 2021 •
edited
Loading

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

@pawelpasterz pawelpasterz enabled auto-merge (squash) April 28, 2021 19:32
@bootstraponline bootstraponline force-pushed the 1758-refactor-fetch-artifacts branch from a900029 to c132475 Compare April 29, 2021 07:37
@Sloox Sloox self-requested a review April 29, 2021 14:51
@pawelpasterz pawelpasterz force-pushed the 1758-refactor-fetch-artifacts branch from c132475 to 98643f9 Compare April 29, 2021 15:53
@bootstraponline bootstraponline force-pushed the 1758-refactor-fetch-artifacts branch 2 times, most recently from f3b51a9 to ee67805 Compare April 30, 2021 12:03
@pawelpasterz pawelpasterz force-pushed the 1758-refactor-fetch-artifacts branch 3 times, most recently from 8b5f41a to fcddceb Compare May 3, 2021 19:03
@pawelpasterz pawelpasterz requested a review from zuziaka May 4, 2021 12:06
@pawelpasterz pawelpasterz force-pushed the 1758-refactor-fetch-artifacts branch 2 times, most recently from c775255 to e31dbf5 Compare May 4, 2021 16:02
@pawelpasterz pawelpasterz force-pushed the 1758-refactor-fetch-artifacts branch 4 times, most recently from 73775e4 to 9ec50d3 Compare May 6, 2021 10:57
@pawelpasterz pawelpasterz force-pushed the 1758-refactor-fetch-artifacts branch from 9ec50d3 to 144d052 Compare May 6, 2021 15:26
@pawelpasterz pawelpasterz merged commit ebc7c02 into master May 6, 2021
@pawelpasterz pawelpasterz deleted the 1758-refactor-fetch-artifacts branch May 6, 2021 15:58
@github-actions github-actions bot locked and limited conversation to collaborators May 6, 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

@adamfilipow92 adamfilipow92 adamfilipow92 approved these changes

@piotradamczyk5 piotradamczyk5 Awaiting requested review from piotradamczyk5

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

@zuziaka zuziaka Awaiting requested review from zuziaka

Assignees

@pawelpasterz pawelpasterz

Labels
Refactor Tiger 🐯
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Data scratch - artifacts
4 participants
@pawelpasterz @Sloox @jan-goral @adamfilipow92