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

Removes the sdk generation tooling from our main build image #630

Merged
merged 1 commit into from
Apr 14, 2019
Merged

Removes the sdk generation tooling from our main build image #630

merged 1 commit into from
Apr 14, 2019

Conversation

cyriltovena
Copy link
Collaborator

@cyriltovena cyriltovena commented Mar 1, 2019 •
edited
Loading

This removes all tooling related to building and generating code for each supported sdk.
We now support 3 commands per SDK (build,test,gen).
I updated the documentation on how to add a new SDK.

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: 86a6eae2-403f-424c-bf33-af1995c058f7

To get permission to view the Cloud Build view, join the agones-discuss Google Group.

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: 78ca25df-6b2e-4c15-b7a9-07f6e2a1c820

To get permission to view the Cloud Build view, join the agones-discuss Google Group.

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: b1b93a84-ed1f-4f33-ae54-e5967335aac3

To get permission to view the Cloud Build view, join the agones-discuss Google Group.

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: bfd1ef41-2d85-4c8d-9c5c-43cab06e51d3

To get permission to view the Cloud Build view, join the agones-discuss Google Group.

@cyriltovena cyriltovena marked this pull request as ready for review March 20, 2019 01:42
@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: 056e272c-7889-4eaf-b09c-35a0e8bc20e1

To get permission to view the Cloud Build view, join the agones-discuss Google Group.

@cyriltovena cyriltovena force-pushed the refactor/sdk-pipeline branch 2 times, most recently from 96e35ad to e06da5a Compare March 20, 2019 02:09
@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: e73b8be9-fc70-4aba-b8a3-7173353dd9ce

To get permission to view the Cloud Build view, join the agones-discuss Google Group.

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: 07614fce-4ca6-4e13-abfd-919e5bbe6212

To get permission to view the Cloud Build view, join the agones-discuss Google Group.

cyriltovena
cyriltovena commented Mar 24, 2019
View reviewed changes
build/Makefile Outdated Show resolved Hide resolved
@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: dd8e97c4-ef55-4f32-b0a2-05d4644d63e4

To get permission to view the Cloud Build view, join the agones-discuss Google Group.

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: 5b556a34-3243-4831-83cb-4e8ce31fbd47

The following development artifacts have been built, and will exist for the next 30 days:

A preview of the website (the last 30 builds are retained):

To install this version:

  • git fetch https://github.com/GoogleCloudPlatform/agones.git pull/630/head:pr_630 && git checkout pr_630
  • helm install install/helm/agones --namespace agones-system --name agones --set agones.image.tag=0.9.0-d3a0417

@cyriltovena
Copy link
Collaborator Author

@markmandel this is ready to review.

markmandel
markmandel reviewed Mar 26, 2019
View reviewed changes
Copy link
Member

@markmandel markmandel left a comment

Choose a reason for hiding this comment

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

Overall, really like the approach! Just have a few questions (and curious if the move to CMake will break your things on the C++ side)

build/build-sdk-images/README.md Show resolved Hide resolved
build/includes/sdk.mk Show resolved Hide resolved
@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: 0c301403-9f63-4667-bbd5-bab1facddbb9

To get permission to view the Cloud Build view, join the agones-discuss Google Group.

@markmandel markmandel added area/build-tools Development tooling. I.e. pretty much everything in the `build` directory. kind/cleanup Refactoring code, fixing up documentation, etc labels Mar 26, 2019
@cyriltovena
Copy link
Collaborator Author

I will look into the new CPP.

@markmandel markmandel added the feature-freeze-do-not-merge Only eligible to be merged once we are out of feature freeze (next full release) label Mar 27, 2019
@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: 7eb94e17-3855-4819-a224-b15448fc791b

To get permission to view the Cloud Build view, join the agones-discuss Google Group.

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: a8f9bd4c-b0bc-4323-9c6a-e2fdb2ec0fa1

To get permission to view the Cloud Build view, join the agones-discuss Google Group.

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: 5d6ccc9b-a129-433f-ae93-9176cf010513

To get permission to view the Cloud Build view, join the agones-discuss Google Group.

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: 33ea0229-5972-4181-a534-9065a6ab2552

To get permission to view the Cloud Build view, join the agones-discuss Google Group.

@googlebot
Copy link

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: d6b1b2e8-6bd6-4de3-b649-17c9c43a3a6f

To get permission to view the Cloud Build view, join the agones-discuss Google Group.

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: 5fd030e0-7a97-4871-acd7-f2682e039f3b

To get permission to view the Cloud Build view, join the agones-discuss Google Group.

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: 201ac76e-40e1-4fda-8957-814830075acd

The following development artifacts have been built, and will exist for the next 30 days:

A preview of the website (the last 30 builds are retained):

To install this version:

  • git fetch https://github.com/GoogleCloudPlatform/agones.git pull/630/head:pr_630 && git checkout pr_630
  • helm install install/helm/agones --namespace agones-system --name agones --set agones.image.tag=0.9.0-069561d

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@cyriltovena cyriltovena force-pushed the refactor/sdk-pipeline branch 2 times, most recently from 3a8ad8e to df9ef72 Compare April 3, 2019 22:52
@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: 908ab2e9-139c-4c70-9cea-b6d4cd6c3d52

To get permission to view the Cloud Build view, join the agones-discuss Google Group.

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: 8c497a6f-f766-4409-ae02-1196f59c5934

To get permission to view the Cloud Build view, join the agones-discuss Google Group.

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: 617d3dbe-eb48-4d05-8d1a-e2f7ca845357

The following development artifacts have been built, and will exist for the next 30 days:

A preview of the website (the last 30 builds are retained):

To install this version:

  • git fetch https://github.com/GoogleCloudPlatform/agones.git pull/630/head:pr_630 && git checkout pr_630
  • helm install install/helm/agones --namespace agones-system --name agones --set agones.image.tag=0.10.0-df9ef72

@markmandel markmandel removed feature-freeze-do-not-merge Only eligible to be merged once we are out of feature freeze (next full release) labels Apr 12, 2019
@markmandel
Copy link
Member

Just letting you know, I have space again to review this. But there are a bunch of conflicts now :(

But I'll pull this down local and take it for a spin shortly!

markmandel
markmandel reviewed Apr 13, 2019
View reviewed changes
Copy link
Member

@markmandel markmandel left a comment

Choose a reason for hiding this comment

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

Just ran though this locally, and it looks good! The rebuild locally on test is fine, but it is a bit slow on Cloud Build due to the lack of that cache.

To that end, I'm happy to approve this - but the only change I want to see that it doesn't close #599

In follow up PR's I'd like to see some CI optimisations - such as doing a pull/push on the SDK images to GCR.io - much like what we do here: https://github.com/GoogleCloudPlatform/agones/blob/df9ef72856cc9b5ea355780d0296f51a46382332/build/Makefile#L417-L418

Which will mean we don't have to rebuild each SDK image on every run - and I think then #599 can be closed.

Does that sound good?

This was referenced Apr 13, 2019
Closed
Closed
Removes the sdk generation tooling from our main build image.
0f56281 
Each supported SDK language has its own Dockerfile.
We support 3 commands (gen,build,test) for each SDK.
Add documentation on how to add a new SDK.
@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: a0047b54-ec51-4118-b243-b5a9a65ac725

The following development artifacts have been built, and will exist for the next 30 days:

A preview of the website (the last 30 builds are retained):

To install this version:

  • git fetch https://github.com/GoogleCloudPlatform/agones.git pull/630/head:pr_630 && git checkout pr_630
  • helm install install/helm/agones --namespace agones-system --name agones --set agones.image.tag=0.10.0-0f56281

markmandel
markmandel approved these changes Apr 14, 2019
View reviewed changes
Copy link
Member

@markmandel markmandel left a comment

Choose a reason for hiding this comment

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

👍

@markmandel markmandel merged commit 0bc29df into googleforgames:master Apr 14, 2019
@heartrobotninja heartrobotninja mentioned this pull request Apr 16, 2019
Merged
@markmandel markmandel added this to the 0.10.0 milestone May 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@markmandel markmandel markmandel approved these changes

Assignees
No one assigned
Labels
area/build-tools Development tooling. I.e. pretty much everything in the `build` directory. kind/cleanup Refactoring code, fixing up documentation, etc
Projects
None yet
Milestone
0.10.0
Development

Successfully merging this pull request may close these issues.
4 participants
@cyriltovena @agones-bot @googlebot @markmandel