Skip to content

Ci build #15

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

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Ci build #15

wants to merge 8 commits into from

Conversation

dmarx
Copy link
Contributor

@dmarx dmarx commented Sep 14, 2022

re-build protobuf stubs on change.

to do:

  • workflow should push a PR to the SDK repo (or maybe SDK workflow should cron trigger?)
  • make sure workflow trigger makes sense

cmake --build .
- name: Commit files
run: |
git config --local user.name "dmarx"
Copy link
Contributor

Choose a reason for hiding this comment

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

This should not override the default action user.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this will run into the same issue described in the sdk CI: pretty sure if I don't configure a user, it'll just complain. How do we make sure a default action user is configured? or should we just choose a consistent username for gh actions?

Copy link
Contributor

Choose a reason for hiding this comment

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

Same as there:

You can use github.actor from the github context https://docs.github.com/en/actions/learn-github-actions/contexts#github-context.

- name: CMake & Build
run: |
cmake .
cmake --build .
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to run the clean target first as sometimes it seems to skip some files during build, probably something that can be fixed with the cmake configuration but until it is need to clean before build.

node-version: "16"
#- name: Install additional tooling
# run: |
# sudo apt install -y protobuf-compiler-grpc libgrpc6 libgrpc++1
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove these commented out items.

run: |
git config --local user.name "dmarx"
git add ./gooseai/*
git commit -m "Updated protobuf files"
Copy link
Contributor

Choose a reason for hiding this comment

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

Possibly could use a better commit message as this sounds more like updating the proto files to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yup, that was a copypaste from the sdk CI, my b

name: Build protobuf stubs

on:
push:
Copy link
Contributor

Choose a reason for hiding this comment

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

If left on all branches then it will regenerate in work branches which I don't think is ideal.

@@ -1,6 +1,6 @@
module github.com/stability-ai/api-interfaces/gooseai

go 1.18
go 1.17
Copy link
Contributor

Choose a reason for hiding this comment

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

We want to keep this at go 1.18

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah I have no idea why that changed and it freaked me out a bit when I first saw it. might need to force a version in one of the CI actions

@@ -1,7 +1,7 @@
// Code generated by protoc-gen-go. DO NOT EDIT.
// versions:
// protoc-gen-go v1.26.0
// protoc v3.21.4
// protoc v3.12.2
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like this is using the older protoc, would like to stay with the more recent version.

Copy link
Contributor

@arsenetar arsenetar left a comment

Choose a reason for hiding this comment

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

Some cleanup as commented, and needs to run clean before build.

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@palp palp mentioned this pull request Sep 27, 2022
@wbrown
Copy link
Contributor

wbrown commented Sep 28, 2022

@dmarx Is this OBE? Are we goin to address @arsenetar's comments?

@wbrown
Copy link
Contributor

wbrown commented Oct 25, 2022

@dmarx Ping.

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.

3 participants