Skip to content
This repository has been archived by the owner on Mar 27, 2024. It is now read-only.

Add GitHub actions #197

Merged
merged 6 commits into from
Jul 21, 2022
Merged

Add GitHub actions #197

merged 6 commits into from
Jul 21, 2022

Conversation

AlyxPractice
Copy link
Contributor

While playing with Bufallo, I noticed Github Actions was not supported out-of-the box. This PR aims to solve this by adding a standard GHA template.

I've sorted the ci-provider alphabetically, along the tests, so it's easier to spot what we are looking for.

I haven't had the chance yet to test the template itself.

I'm fairly new to Golang so I might have missed a few things, let me know.


Note
There were some issues with tests that I don't think my changes are touching, I'm not sure what's happening.

@AlyxPractice AlyxPractice requested a review from a team as a code owner July 17, 2022 19:19
@sio4 sio4 self-assigned this Jul 18, 2022
@sio4 sio4 added the enhancement New feature or request label Jul 18, 2022
Copy link
Member

@sio4 sio4 left a comment

Choose a reason for hiding this comment

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

Thanks for adding Github Action!
Also, sorting them on the option is nice. (just one of my habits, I usually make a separate commit for easy reading for changed things on another commit but yours also fine :-)

I added some comments about unnecessary changes in the test code and an outdated command. Please fix them and I will proceed the next step.

internal/genny/add/add_test.go Outdated Show resolved Hide resolved
internal/genny/plugins/install/install_test.go Outdated Show resolved Hide resolved
@AlyxPractice AlyxPractice requested a review from sio4 July 19, 2022 19:52
@AlyxPractice
Copy link
Contributor Author

Thanks a lot for your quick review! Let me know if I can do something else.

@sio4
Copy link
Member

sio4 commented Jul 20, 2022

@VictorBersy please do not push an empty commit.

@AlyxPractice
Copy link
Contributor Author

@VictorBersy please do not push an empty commit.

Sorry I don't think I can restart failing jobs :(

Won't it be squashed anyway at the merge?

@sio4
Copy link
Member

sio4 commented Jul 20, 2022

Yeah, will do. anyway, if I found a failed test, I still have a chance to check if the failure is ignorable or just run the failed one manually without spending the whole test hours. also, I need to check/review what is on the (empty) commit. :-)

@AlyxPractice
Copy link
Contributor Author

Oh right I see 🙇 It's my first real OSS contributions so I'm still figuring out things 🫣

@sio4
Copy link
Member

sio4 commented Jul 20, 2022

NOTE: failed test is not an issue with the PR but the timeout. (need to check if we can reduce the testing time)

  • panic: test timed out after 10m0s
  • cli/internal/cmd/fix_test.TestFix_v0_16_27.func2

@sio4
Copy link
Member

sio4 commented Jul 20, 2022

I roughly don't think this PR introduced the test failure but I would like to check the fix manually.

Please wait until the test is completed.

@sio4
Copy link
Member

sio4 commented Jul 21, 2022

I tested the failed test case (actually along with all others) in my local environment (ubuntu 20.04 + go1.17.8) and the result was fine. I will merge it into the development branch now and will check if the test fails again.

Interestingly, the other PRs (dependabot's PRs) just succeeded on the same tests always 🤔

@sio4 sio4 merged commit fc849e4 into gobuffalo:development Jul 21, 2022
@AlyxPractice
Copy link
Contributor Author

Interesting, I will try to see if I can help with #200 then.

Glad I could contribute, thanks a lot for your patience and review 🙇

@AlyxPractice AlyxPractice deleted the add-github-actions branch July 21, 2022 11:00
@sio4
Copy link
Member

sio4 commented Jul 21, 2022

Thank you for your contribution and your patience during waiting for a long review. I wanted to make it sure the change is totally compatible with the code base and also it could be a chance to find/fix another (unrelated) bug on the code or testing.

Interesting, I will try to see if I can help with #200 then.

I think there should be some strategic decision inside the team, and since there is another progress updating plugin architecture which is a big thing and not fully compatible with the current code base, I think the issue would be addressed once we have a new codebase and having discussion.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants