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

Fix ensure SDK image #878

Merged
merged 1 commit into from
Jul 24, 2019
Merged

Conversation

aLekSer
Copy link
Collaborator

@aLekSer aLekSer commented Jul 5, 2019

Use bash scripts hash instead of build image version.
Does not rebuild SDK images if image was found, previously rebuild was performed every time.

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: 8d166230-5023-4949-95a8-f1b958cb8145

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

@aLekSer
Copy link
Collaborator Author

aLekSer commented Jul 5, 2019

E2E test failed, not connected with this PR:

Step #17: time="2019-07-05 20:22:03.977" level=info msg="Scaling fleet" fleet=preferred-gngb4 patch="[{ \"op\": \"replace\", \"path\": \"/spec/replicas\", \"value\": 120 }]" scale=120
Step #17: time="2019-07-05 20:22:04.575" level=info msg="fleet simple-fleet-8mlkz has 5/3 ready replicas"
Step #17: --- FAIL: TestAutoscalerStressCreate (68.99s)
Step #17: fleetautoscaler_test.go:169: 
Step #17: Error Trace:	fleetautoscaler_test.go:169
Step #17: fleetautoscaler_test.go:191
Step #17: Error: Should be true
Step #17: Test: TestAutoscalerStressCreate
Step #17: Messages: FleetAutoscaler created even if the parameters are NOT valid: 4 3 5
Step #17: fleetautoscaler_test.go:169: 
Step #17: Error Trace:	fleetautoscaler_test.go:169
Step #17: fleetautoscaler_test.go:191
Step #17: Error: Should be true
Step #17: Test: TestAutoscalerStressCreate
Step #17: Messages: FleetAutoscaler created even if the parameters are NOT valid: 3 1 2

build/includes/sdk.mk Outdated Show resolved Hide resolved
@aLekSer aLekSer force-pushed the ensure-sdk-image branch from 3517e8f to fd4fb7a Compare July 15, 2019 14:07
@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: 2d9dba8a-23e3-4092-89e4-95cbc9ce2796

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

@aLekSer aLekSer force-pushed the ensure-sdk-image branch 2 times, most recently from 8914a45 to e76b368 Compare July 15, 2019 14:41
@aLekSer
Copy link
Collaborator Author

aLekSer commented Jul 15, 2019

Added crossplatform sha_bash function verified on Linux and MacOs. Currently on both it gives a result of 5e38cff19f.
This make target will let you see the sdk version (not included in PR):

version: 
	echo '$(build_sdk_version)'

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: 076ce844-edd9-4d27-b5be-f7f535c5c203

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/878/head:pr_878 && git checkout pr_878
  • helm install install/helm/agones --namespace agones-system --name agones --set agones.image.tag=0.12.0-8914a45

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: 04fe0353-5679-473f-bd94-59c5dd6fbad0

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/878/head:pr_878 && git checkout pr_878
  • helm install install/helm/agones --namespace agones-system --name agones --set agones.image.tag=0.12.0-e76b368

@@ -22,12 +22,14 @@

build_sdk_base_version = $(call sha,$(build_path)/build-sdk-images/tool/base/Dockerfile)
build_sdk_base_tag = agones-build-sdk-base:$(build_sdk_base_version)
build_sdk_version = $(call sha_bash,$(build_path)/build-sdk-images/*/*.sh)
Copy link
Member

Choose a reason for hiding this comment

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

A couple of questions on this:

  1. What if I change the /build-sdk-images/go/Dockerfile then this won't rebuild, since it's only looking at .sh files, no?
  2. If i change /build-sdk-images/go/gen.sh - won't that mean that all images will get rebuilt?

Should this be (or something close to):

Suggested change
build_sdk_version = $(call sha_bash,$(build_path)/build-sdk-images/*/*.sh)
build_sdk_version = $(call sha_bash,$(build_path)/build-sdk-images/$(SDK_FOLDER)/*)

?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nice questions, I don't expect these scripts to be changed too often.
Currently we have rebuild every time. So this version is better, but not absolutely. Let me check both questions in practise. But I think:

  1. We would have another base-image so rebuild would be triggered.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry I'm confused 😕 , if I only change /build-sdk-images/go/Dockerfile - then the image won't rebuild, it would only change if I changed the .sh files. This could be really confusing for the developer.

I can't see a path that would cause that to do anything else? But I could be missing something.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You are right definitely, let's concat those two hashes. Will resend new version in a moment.

Copy link
Member

Choose a reason for hiding this comment

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

I think we might be saying two different, but important things.

I think you are referencing when the /build-sdk-images/tool/base/Dockerfile changes.

I'm talking about when the language specific Dockerfile changes, for example /build-sdk-images/go/Dockerfile

You are right - both are actually issues!

Copy link
Member

Choose a reason for hiding this comment

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

Just looking at this again, does this bring us back to the original problems?

@aLekSer aLekSer force-pushed the ensure-sdk-image branch from e76b368 to a7df0a5 Compare July 16, 2019 19:58
@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: accb4f21-00dc-4ea1-9774-dc83b2bcbe31

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/878/head:pr_878 && git checkout pr_878
  • helm install install/helm/agones --namespace agones-system --name agones --set agones.image.tag=0.12.0-a7df0a5

@@ -22,12 +22,14 @@

build_sdk_base_version = $(call sha,$(build_path)/build-sdk-images/tool/base/Dockerfile)
build_sdk_base_tag = agones-build-sdk-base:$(build_sdk_base_version)
build_sdk_version = $(call sha_bash,$(build_path)/build-sdk-images/$(SDK_FOLDER)/*.sh)$(call sha,$(build_path)/build-sdk-images/$(SDK_FOLDER)/Dockerfile)
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
build_sdk_version = $(call sha_bash,$(build_path)/build-sdk-images/$(SDK_FOLDER)/*.sh)$(call sha,$(build_path)/build-sdk-images/$(SDK_FOLDER)/Dockerfile)
build_sdk_version = $(call sha_bash,$(build_path)/build-sdk-images/$(SDK_FOLDER)/{*.sh,Dockerfile})

sha256sum {*.sh,Dockerfile} | cut -d' ' -f1 | sha256sum | head -c 10 - works in bash!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks, let's use just $(SDK_FOLDER)/* because suggested version leads to :

sha256sum: '"/home/UserName/go/src/agones.dev/agones/build//build-sdk-images/go/{*.sh': No such file or directory

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I mean we could even more simplify to:

sha256sum {*} | cut -d' ' -f1 | sha256sum | head -c 10

Copy link
Member

Choose a reason for hiding this comment

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

Yes - we could use everything in the directory. We may end up with a README.md in each one, but that's probably fine if we happen to catch that. WDYT?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, the proper wildcard should fix README.md case in the future.

Copy link
Member

Choose a reason for hiding this comment

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

I think it's fine if we end up pickup up a README - also it's not an issue right now so, let's just use a solution that simplest for now (use build_sdk_version = $(call sha_bash,$(build_path)/build-sdk-images/$(SDK_FOLDER)/*) and we can always revisit it when we actually need to.

WDYT?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, that would work, for now we don't have README files in any of build-sdk-images/$(SDK_FOLDER)/, I will update PR with suggested naming.

@aLekSer aLekSer force-pushed the ensure-sdk-image branch 2 times, most recently from 05e055f to e1b80c7 Compare July 16, 2019 21:29
@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: 27fb7373-5900-49bd-9d9d-abccb1193d54

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

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: 148436d5-2006-42d6-a008-4fc5ce728849

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

@aLekSer aLekSer force-pushed the ensure-sdk-image branch from e1b80c7 to dd4eebc Compare July 17, 2019 08:09
@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: 4724ba3b-c7d6-4b55-a4d8-e429bafdfd0d

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

@aLekSer aLekSer force-pushed the ensure-sdk-image branch 3 times, most recently from 9d24d11 to b8aa277 Compare July 22, 2019 10:14
@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: 919e83a0-d3c9-4e11-a480-5a55f8c28830

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/878/head:pr_878 && git checkout pr_878
  • helm install install/helm/agones --namespace agones-system --name agones --set agones.image.tag=0.12.0-6541bf6

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: 559af4a0-6e50-4266-b0b1-0057e646a708

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/878/head:pr_878 && git checkout pr_878
  • helm install install/helm/agones --namespace agones-system --name agones --set agones.image.tag=0.12.0-9d24d11

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: a8ab8949-2635-4315-991f-50820435a946

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

@aLekSer aLekSer force-pushed the ensure-sdk-image branch from b8aa277 to b9de525 Compare July 22, 2019 12:07
@aLekSer
Copy link
Collaborator Author

aLekSer commented Jul 22, 2019

Build failed because I mistakenly added reserve check into all tests.

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: ae57bb38-8e4c-4cb9-b9e5-7a8ad08178ad

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/878/head:pr_878 && git checkout pr_878
  • helm install install/helm/agones --namespace agones-system --name agones --set agones.image.tag=0.12.0-b9de525

@@ -22,12 +22,14 @@

build_sdk_base_version = $(call sha,$(build_path)/build-sdk-images/tool/base/Dockerfile)
build_sdk_base_tag = agones-build-sdk-base:$(build_sdk_base_version)
build_sdk_version = $(call sha_dir,$(build_path)/build-sdk-images/$(SDK_FOLDER)/*.sh)
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
build_sdk_version = $(call sha_dir,$(build_path)/build-sdk-images/$(SDK_FOLDER)/*.sh)
build_sdk_version = $(call sha_dir,$(build_path)/build-sdk-images/$(SDK_FOLDER)/*)

Yes?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes, sorry, fixing

@aLekSer aLekSer force-pushed the ensure-sdk-image branch from b9de525 to 1a8ed89 Compare July 23, 2019 10:18
@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: 94e276d5-660a-4557-9394-f8ee50eb3cfc

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/878/head:pr_878 && git checkout pr_878
  • helm install install/helm/agones --namespace agones-system --name agones --set agones.image.tag=0.12.0-1a8ed89

@aLekSer aLekSer force-pushed the ensure-sdk-image branch from 1a8ed89 to 33a5a48 Compare July 23, 2019 12:38
@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: 8fffef33-0477-4ed8-8a1c-921bbf1269a0

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

@aLekSer aLekSer force-pushed the ensure-sdk-image branch from 33a5a48 to 5a24d2e Compare July 23, 2019 13:47
@aLekSer
Copy link
Collaborator Author

aLekSer commented Jul 23, 2019

Updated comment to :
# Get the sha of all files in a directory using wildcard in $(1)

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: 805bbe62-e9b1-4d3d-a3dc-a09f707e72da

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

@aLekSer
Copy link
Collaborator Author

aLekSer commented Jul 23, 2019

Strange flaky test not connected with this changes:

--- FAIL: TestControllerApplyGameServerAddressAndPort (0.15s)
controller_test.go:916: 
Error Trace:	controller_test.go:916
Error: Expected nil, but got: error getting external address for GameServer test: error retrieving node node1 for Pod test: node "node1" not found
Test: TestControllerApplyGameServerAddressAndPort
panic: runtime error: index out of range [recovered]
panic: runtime error: index out of range

@aLekSer aLekSer force-pushed the ensure-sdk-image branch from 5a24d2e to d0e8715 Compare July 23, 2019 14:25
@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: 12db66dc-218b-46fa-9018-641ee6d47e99

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/878/head:pr_878 && git checkout pr_878
  • helm install install/helm/agones --namespace agones-system --name agones --set agones.image.tag=0.12.0-d0e8715

Use git version instead of build image version.
Does not rebuild SDK images if image was found.
@aLekSer aLekSer force-pushed the ensure-sdk-image branch from d0e8715 to dc54924 Compare July 23, 2019 16:01
@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: cc26f111-1d64-4103-8e20-713bccdbef03

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/878/head:pr_878 && git checkout pr_878
  • helm install install/helm/agones --namespace agones-system --name agones --set agones.image.tag=0.12.0-dc54924

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 4906737 into googleforgames:master Jul 24, 2019
@markmandel markmandel added area/tests Unit tests, e2e tests, anything to make sure things don't break kind/cleanup Refactoring code, fixing up documentation, etc labels Jul 24, 2019
@markmandel markmandel added this to the 0.12.0 milestone Jul 24, 2019
@aLekSer aLekSer deleted the ensure-sdk-image branch July 24, 2019 06:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/tests Unit tests, e2e tests, anything to make sure things don't break kind/cleanup Refactoring code, fixing up documentation, etc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants