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

workflow to run go test #15

Merged
merged 4 commits into from
Jun 29, 2024
Merged

workflow to run go test #15

merged 4 commits into from
Jun 29, 2024

Conversation

daveads
Copy link
Contributor

@daveads daveads commented Apr 9, 2024

Description

Set up workflow to run golang test using "make"

This issue >> #8

Copy link

github-actions bot commented Apr 9, 2024

Thank you for the pull request!

The Scribe team will do our best to address your contribution as soon as we can. The following is a checklist for maintainers to make sure this process goes as well as possible. Feel free to address the points below yourself in further commits if you realize that actions are needed :)

If you're not already a member of our public Matrix community, please consider joining! It'd be great to have you!

Maintainer checklist

  • The commit messages for the remote branch should be checked to make sure the contributor's email is set up correctly so that they receive credit for their contribution

    • The contributor's name and icon in remote commits should be the same as what appears in the PR
    • If there's a mismatch, the contributor needs to make sure that the email they use for GitHub matches what they have for git config user.email in their local activist repo
  • The CHANGELOG has been updated with a description of the changes for the upcoming release and the corresponding issue (if necessary)

@daveads
Copy link
Contributor Author

daveads commented Apr 21, 2024

@wkyoshida @wkyoshida

Copy link
Member

@wkyoshida wkyoshida left a comment

Choose a reason for hiding this comment

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

Hey @daveads!
Thank you for your patience with my review 😅 Apologies for the wait
I added a couple thoughts if you have interest in taking a look, but I can also make the changes myself, if not - no worries
Thanks again!

- name: Setup Go
uses: actions/setup-go@v4
with:
go-version: '1.22.2'
Copy link
Member

Choose a reason for hiding this comment

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

The go version that Scribe-Server uses is 1.20 (as in the go.mod file). This is due to the version that was available in Toolforge, where Scribe-Server is hosted.
We should probably test against the same version that Scribe-Server is on 🤔 That got me thinking though.. could we get this action to read the version off the go.mod file in case we change the version? I checked the docs and lo and behold, it looks like we can! 🚀 Doing this would be great

Comment on lines 23 to 24
- name: Build
run: make build
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- name: Build
run: make build

We can probably remove this build step for now actually. Running the tests is probably sufficient here.
Eventually, we could have some automation for the build step, but it probably makes sense for that to be a separate future workflow.

@@ -0,0 +1,24 @@
name: CI
Copy link
Member

Choose a reason for hiding this comment

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

Thinking here that we could name this workflow in the same way that we did for Scribe-Data actually, pr_ci.yaml
We could name this pr_ci as well and the file pr_ci.yaml

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay

@wkyoshida wkyoshida merged commit c32c04a into scribe-org:main Jun 29, 2024
1 check passed
@wkyoshida
Copy link
Member

I made the final change to read the go version from the go.mod file using the go-version-file option and merged the PR. Thanks for the contribution, @daveads! 🚀

@wkyoshida wkyoshida mentioned this pull request Jun 29, 2024
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants