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

Feature/524 download acceptance assets #550

Merged
merged 5 commits into from
Apr 8, 2020
Merged

Feature/524 download acceptance assets #550

merged 5 commits into from
Apr 8, 2020

Conversation

natalieparellano
Copy link
Member

@natalieparellano natalieparellano commented Mar 30, 2020 •
edited
Loading

Fixes #524

@natalieparellano
Copy link
Member Author

This is not ready to be merged but potentially could use some high-level feedback on the approach as I'm ironing out the issues.

@natalieparellano
Copy link
Member Author

Our linter is not happy. It thinks code is unused that is actually used :/

@natalieparellano
Copy link
Member Author

I'm able to run the combo suite with just COMPILE_PACK_WITH_VERSION=0.0.0 make acceptance-all - yay!

I wonder if (when we're happy with this) we could switch the GitHub actions workflow to this way of running things, so that it's implicitly tested.

@jromero
Copy link
Member

jromero commented Mar 31, 2020

I wonder if (when we're happy with this) we could switch the GitHub actions workflow to this way of running things, so that it's implicitly tested.

I'd like that generally. The only drawback would be running not running each combination in parallel would greatly increase testing time. Plus, the overall output/logs might be harder to consume. Things to consider.

@natalieparellano
Copy link
Member Author

I wonder if (when we're happy with this) we could switch the GitHub actions workflow to this way of running things, so that it's implicitly tested.

I'd like that generally. The only drawback would be running not running each combination in parallel would greatly increase testing time. Plus, the overall output/logs might be harder to consume. Things to consider.

We could have 4 different workflows with different values for ACCEPTANCE_SUITE_CONFIG. I'm also realizing that we'd want to add authentication to avoid getting rate limited. But I think it's doable and potentially worth it.

@natalieparellano natalieparellano force-pushed the feature/524-download-acceptance-assets branch 3 times, most recently from fe49034 to 86a2cd9 Compare April 1, 2020 19:06
@natalieparellano
Copy link
Member Author

I am not sure why the linter is unhappy - maybe we're running into this bug? golangci/golangci-lint#791 ...the code it thinks is unused is actually used!

@natalieparellano natalieparellano marked this pull request as ready for review April 1, 2020 19:16
@natalieparellano natalieparellano requested a review from a team as a code owner April 1, 2020 19:16
jromero
jromero suggested changes Apr 2, 2020
View reviewed changes
Copy link
Member

@jromero jromero left a comment

Choose a reason for hiding this comment

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

Nice! Left a few comments and questions. Hopefully nothing too involved. Once those are resolved, I'll give it a try and run the tests locally to see how all this performs.

Thank you @natalieparellano! I'm personally looking forward to get this feature in. 😄

acceptance/input_paths_manager.go Show resolved Hide resolved
acceptance/input_paths_manager.go Outdated Show resolved Hide resolved
acceptance/input_paths_manager.go Show resolved Hide resolved
acceptance/run_combination.go Show resolved Hide resolved
acceptance/input_paths_manager.go Show resolved Hide resolved
acceptance/github_asset_fetcher.go Show resolved Hide resolved
acceptance/github_asset_fetcher.go Show resolved Hide resolved
acceptance/github_asset_fetcher.go Outdated Show resolved Hide resolved
acceptance/github_asset_fetcher.go Show resolved Hide resolved
acceptance/github_asset_fetcher.go Outdated Show resolved Hide resolved
@natalieparellano
Download combo suite dependencies if not provided
8c68492 
- Input paths manager gathers inputs from enviroment and fills in
  missing inputs for the provided run combinations
- GitHub asset fetcher queries GitHub for release versions and assets
- Versions and assets are cached in PACK_HOME
- Re-arrange some setup code in acceptance_test.go to be more readable

Signed-off-by: Natalie Arellano <[email protected]>
@natalieparellano
Update per review comments.
1f16a78 
Signed-off-by: Natalie Arellano <[email protected]>
@natalieparellano natalieparellano force-pushed the feature/524-download-acceptance-assets branch from 86a2cd9 to 1f16a78 Compare April 7, 2020 19:06
@natalieparellano
Remove newlines
f90679d 
They are unnecessary.

Signed-off-by: Natalie Arellano <[email protected]>
@natalieparellano
Copy link
Member Author

@jromero I think this is ready for another round.

@natalieparellano
Update lifecycle expression
e029c63 
We always want the Linux binaries, even on Windows.

Signed-off-by: Natalie Arellano <[email protected]>
@natalieparellano
Use new way of downloading assets in GitHub action workflow
2d1d5b5 
Adds authentication to the GithubAssetFetcher to avoid rate limiting.

Signed-off-by: Natalie Arellano <[email protected]>
@natalieparellano natalieparellano mentioned this pull request Apr 8, 2020
Closed
@natalieparellano
Copy link
Member Author

natalieparellano commented Apr 8, 2020 •
edited
Loading

@jromero updated GitHub actions workflow to download assets using the new code (as described in #565).

@jromero jromero changed the base branch from master to release/0.10.0 April 8, 2020 21:49
@jromero jromero merged commit 9897ca6 into release/0.10.0 Apr 8, 2020
@jromero jromero deleted the feature/524-download-acceptance-assets branch April 8, 2020 21:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jromero jromero jromero approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@natalieparellano @jromero