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

Adding git actions #20

Merged
merged 4 commits into from
Jul 24, 2020
Merged

Conversation

ranakan19
Copy link
Contributor

@ranakan19 ranakan19 commented Jul 23, 2020

added GitActions to do the following:

  • get dependencies
  • check build
  • run tests
  • check format
  • check go mod status

Fixes #11, #17

ranakan19 and others added 2 commits July 23, 2020 18:45
added GitActions to do the following:
- get dependencies
- check build
- run tests
- check format
- check go mod status
@ranakan19 ranakan19 changed the title Ranakan19 git actions Adding git actions Jul 23, 2020
Copy link
Collaborator

@wtam2018 wtam2018 left a comment

Choose a reason for hiding this comment

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

parser (binary file) got committed. Perhaps, we can create GiHub Actions to check for binary files.

Copy link
Member

@sleshchenko sleshchenko left a comment

Choose a reason for hiding this comment

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

Seems parser binary should be added to gitignore and removed from that PR

.github/workflows/go.yml Outdated Show resolved Hide resolved
Copy link
Member

@sleshchenko sleshchenko left a comment

Choose a reason for hiding this comment

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

.github/workflows/go.yml Outdated Show resolved Hide resolved
- name: Run Go Tests
run: go test ./...

- name: Check format
Copy link
Member

Choose a reason for hiding this comment

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

It's not obvious but as far as I see now - it's better to check go.sum first and them go fmt because go fmt could also add missing dependencies to go.sum. See https://github.com/devfile/parser/runs/905595388?check_suite_focus=true

Copy link
Collaborator

Choose a reason for hiding this comment

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

+1 I ran into that as well.

@wtam2018
Copy link
Collaborator

wtam2018 commented Jul 24, 2020

parser (binary file) got committed. Perhaps, we can create GiHub Actions to check for binary files
Add "parser" to .gitingore should be good enough. #22

@sleshchenko
Copy link
Member

sleshchenko commented Jul 24, 2020

Add "parser" to .gitingore should be good enough.

The question: how that binary was built? If it's needed to manually type a command and specify binary name - anyone is free to name binary as they wish.
If we want to name binary named like parser, or devfile-parser (may be precise a bit) then we can set up Makefile rule to compile with binary with a standardized name which is in gitignore. But it's a bit out of the scope of the current PR.

Copy link
Member

@sleshchenko sleshchenko left a comment

Choose a reason for hiding this comment

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

Approving it not to block PR merging but please address the following issues before merging:

  • binary
  • go dep
  • go.sum and format checks orders

@wtam2018
Copy link
Collaborator

wtam2018 commented Jul 24, 2020

Add "parser" to .gitingore should be good enough.

The question: how that binary was built? If it's needed to manually type a command and specify binary name - anyone is free to name binary as they wish.
If we want to name binary named like parser, or devfile-parser (may be precise a bit) then we can set up Makefile rule to compile with binary with a standardized name which is in gitignore. But it's a bit out of the scope of the current PR.

go build will do it. It looks like the default name is picked up from the go module name.

@wtam2018 wtam2018 merged commit adfa8c9 into devfile:master Jul 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants