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

Graduate CustomFasSyncInterval To Stable #3235

Merged
merged 14 commits into from
Jul 10, 2023

Conversation

Kalaiselvi84
Copy link
Contributor

What type of PR is this?

Uncomment only one /kind <> line, press enter to put that in a new line, and remove leading whitespace from that line:

/kind breaking
/kind bug
/kind cleanup
/kind documentation

/kind feature

/kind hotfix

What this PR does / Why we need it:

Which issue(s) this PR fixes:

Closes #3095

Special notes for your reviewer:

@github-actions github-actions bot added the kind/feature New features for Agones label Jun 26, 2023
@Kalaiselvi84 Kalaiselvi84 marked this pull request as draft June 27, 2023 00:18
@markmandel
Copy link
Member

Looks like the agones-bot got stuck. If you rebase against main and push force over the top, CI should kick off.

@Kalaiselvi84
Copy link
Contributor Author

I did rebase against main, still agones-bot is not running. Am I missing anything here?

@markmandel
Copy link
Member

Well that's weird! I'll have to go look!

@markmandel
Copy link
Member

Ah I see it. One second...

@Kalaiselvi84
Copy link
Contributor Author

can I try creating a new PR?

@markmandel
Copy link
Member

Nah, I broke some things, I just fixed them. Running now!

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: 551afebb-d636-48b0-a07c-2068c7e826dc

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

Comment on lines 282 to 283
utilruntime.FeatureTestMutex.Lock()
defer utilruntime.FeatureTestMutex.Unlock()
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
utilruntime.FeatureTestMutex.Lock()
defer utilruntime.FeatureTestMutex.Unlock()

We can remove all of these -- they are only needed when we want to lock down a feature flag within a test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have removed it from all the test function in this file.

pkg/fleetautoscalers/controller.go Show resolved Hide resolved
@Kalaiselvi84 Kalaiselvi84 marked this pull request as ready for review June 29, 2023 17:29
@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: 34e3c100-683f-4eb5-be93-4428f473b5c1

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

@markmandel
Copy link
Member

Oh yeah, sorry - it should run in the container, so it should all just work. Huh, why do I not see it installing Go in the container - that's odd.

@markmandel
Copy link
Member

Looking at your output, it's missing several steps. Have you changed the .sh file in any way? Running an old version somehow? running on a m2 mac?

This is my output when it runs against main

❯ make gen-api-docs
mkdir -p ~/.kube/
mkdir -p /home/markmandel/workspace/agones/build//.gocache
mkdir -p /home/markmandel/workspace/agones/build//.config/gcloud
mkdir -p ~/.config/helm
mkdir -p ~/.cache/helm
make ensure-image IMAGE_TAG=agones-build:09f8403ba1 BUILD_TARGET=build-build-image
make[1]: Entering directory '/home/markmandel/workspace/agones/build'
make[1]: Leaving directory '/home/markmandel/workspace/agones/build'
docker run -e FILE="/go/src/agones.dev/agones/site/content/en/docs/Reference/agones_crd_api_reference.html" -e VERSION=1.33.0 --rm -i -v /home/markmandel/workspace/agones/build//.config/gcloud:/root/.config/gcloud -v ~/.kube/:/root/.kube -v ~/.config/helm:/root/.config/helm -v ~/.cache/helm:/root/.cache/helm -v /home/markmandel/workspace/agones:/go/src/agones.dev/agones -v /home/markmandel/workspace/agones/build//.gomod:/go/pkg/mod -v /home/markmandel/workspace/agones/build//.gocache:/root/.cache/go-build agones-build:09f8403ba1 bash -c "/go/src/agones.dev/agones/site/gen-api-docs.sh"
+ rm -rf /usr/local/go
+ GO_VERSION=1.15.13
+ cd /usr/local
+ wget -q https://dl.google.com/go/go1.15.13.linux-amd64.tar.gz
+ tar -xzf go1.15.13.linux-amd64.tar.gz
+ rm go1.15.13.linux-amd64.tar.gz
+ export GOPROXY=http://proxy.golang.org
+ GOPROXY=http://proxy.golang.org
+ echo 'using go proxy as a workaround for git.agache.org being down: http://proxy.golang.org'
+ cd /go/src/github.com/ahmetb/gen-crd-api-reference-docs
+ go mod edit --replace=agones.dev/agones@latest=../../../agones.dev/agones/
using go proxy as a workaround for git.agache.org being down: http://proxy.golang.org
+ go build
+ cp /go/src/agones.dev/agones/site/assets/templates/pkg.tpl ./template
+ FILE=/go/src/agones.dev/agones/site/content/en/docs/Reference/agones_crd_api_reference.html
+ VERSION=1.33.0
+ TITLE=/tmp/title.html
+ EXPIRY_DOC=/tmp/expiry.html
+ PUBLISH_DOC=/tmp/publish.html
+ RESULT=/tmp/agones_crd_api_reference.html
+ OLD=/tmp/old_docs.html
+ ./gen-crd-api-reference-docs --config ../../../agones.dev/agones/site/assets/templates/crd-doc-config.json --api-dir ../../../agones.dev/agones/pkg/apis/ --out-file /tmp/agones_crd_api_reference.html
I0629 22:07:16.517182     267 main.go:127] parsing go packages in directory ../../../agones.dev/agones/pkg/apis/
W0629 22:07:40.663014     267 parse.go:239] Ignoring child directory agones.dev/agones/pkg/apis/agones/v1/fuzz_test: No files for pkg "agones.dev/agones/pkg/apis/agones/v1/fuzz_test"
I0629 22:07:41.231152     267 main.go:229] using package=agones.dev/agones/pkg/apis/agones/v1
I0629 22:07:41.231163     267 main.go:229] using package=agones.dev/agones/pkg/apis/allocation/v1
I0629 22:07:41.231167     267 main.go:229] using package=agones.dev/agones/pkg/apis/autoscaling/v1
I0629 22:07:41.231169     267 main.go:229] using package=agones.dev/agones/pkg/apis/multicluster/v1
W0629 22:07:41.278711     267 main.go:423] not found external link source for type k8s.io/apimachinery/pkg/util/intstr.IntOrString
W0629 22:07:41.278823     267 main.go:423] not found external link source for type k8s.io/apimachinery/pkg/util/intstr.IntOrString
W0629 22:07:41.278907     267 main.go:423] not found external link source for type k8s.io/apimachinery/pkg/types.UID
W0629 22:07:41.279026     267 main.go:423] not found external link source for type k8s.io/apimachinery/pkg/types.UID
W0629 22:07:41.279941     267 main.go:423] not found external link source for type k8s.io/apimachinery/pkg/util/intstr.IntOrString
W0629 22:07:41.281033     267 main.go:423] not found external link source for type agones.dev/agones/pkg/apis.SchedulingStrategy
W0629 22:07:41.281311     267 main.go:423] not found external link source for type agones.dev/agones/pkg/apis.SchedulingStrategy
W0629 22:07:41.281718     267 main.go:423] not found external link source for type agones.dev/agones/pkg/apis.SchedulingStrategy
W0629 22:07:41.282457     267 main.go:423] not found external link source for type agones.dev/agones/pkg/apis.SchedulingStrategy
W0629 22:07:41.282865     267 main.go:423] not found external link source for type agones.dev/agones/pkg/apis.SchedulingStrategy
W0629 22:07:41.283132     267 main.go:423] not found external link source for type agones.dev/agones/pkg/apis.SchedulingStrategy
W0629 22:07:41.283804     267 main.go:423] not found external link source for type agones.dev/agones/pkg/apis.SchedulingStrategy
W0629 22:07:41.284821     267 main.go:423] not found external link source for type agones.dev/agones/pkg/apis.SchedulingStrategy
W0629 22:07:41.285176     267 main.go:423] not found external link source for type agones.dev/agones/pkg/apis.SchedulingStrategy
I0629 22:07:41.289015     267 main.go:165] written to /tmp/agones_crd_api_reference.html
+ awk '/\ feature\ publishVersion/{flag=1;next}/\ \/feature/{flag=0}flag' /go/src/agones.dev/agones/site/content/en/docs/Reference/agones_crd_api_reference.html
+ awk '/\+\+\+/ {ok=1} /^$/ {ok=0} {if(ok){print $0}}' /go/src/agones.dev/agones/site/content/en/docs/Reference/agones_crd_api_reference.html
+ printf '\n'
++ grep 'feature publishVersion=' /go/src/agones.dev/agones/site/content/en/docs/Reference/agones_crd_api_reference.html
+ doc_version='{{% feature publishVersion="1.33.0" %}}'
+ echo '{{%' feature 'publishVersion="1.33.0"' '%}}'
+ publish='{{% feature publishVersion="1.33.0" %}}'
{{% feature publishVersion="1.33.0" %}}
+ expiry='{{% feature expiryVersion="1.33.0" %}}'
+ sed '/\ expiryVersion="1.33.0"/,/%\ \/feature/!d;/%\ \/feature/q' /go/src/agones.dev/agones/site/content/en/docs/Reference/agones_crd_api_reference.html
+ sed '/\ publishVersion=/,/%\ \/feature/!d;/%\ \/feature/q' /go/src/agones.dev/agones/site/content/en/docs/Reference/agones_crd_api_reference.html
++ sort /tmp/agones_crd_api_reference.html
+ diff /dev/fd/63 /dev/fd/62
++ sort /tmp/old_docs.html

There should be a bunch more commands being run after wget -q https://dl.google.com/go/go1.15.13.linux-amd64.tar.gz

@Kalaiselvi84
Copy link
Contributor Author

I haven't updated anything in the .sh file. The command fails to work on both my main branch and feature branch.

@markmandel
Copy link
Member

I just ran the command on a clean machine (no build image, etc), and it all works correctly. So I'm not sure what to suggest 🤔

@Kalaiselvi84
Copy link
Contributor Author

Kalaiselvi84 commented Jun 30, 2023

I just ran the command on a clean machine (no build image, etc), and it all works correctly. So I'm not sure what to suggest 🤔

There was an issue in load metadata for docker.io/library/alpine:3.14. After executing the command sudo systemctl restart docker to restart the Docker service, it appears that the issue related to loading metadata for the alpine:3.14 image and making requests to the Docker registry (registry-1.docker.io) has been resolved. make gen-api-docs command is now working fine.

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: dc4eb2c6-2852-49d1-a0a9-07b5c4f5ab19

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/googleforgames/agones.git pull/3235/head:pr_3235 && git checkout pr_3235
  • helm install agones ./install/helm/agones --namespace agones-system --set agones.image.registry=us-docker.pkg.dev/agones-images/ci --set agones.image.tag=1.33.0-dev-66fe7b9-amd64

@Kalaiselvi84 Kalaiselvi84 marked this pull request as ready for review June 30, 2023 03:49
@google-oss-prow google-oss-prow bot requested a review from aLekSer June 30, 2023 03:49
@markmandel
Copy link
Member

There was an issue in load metadata for docker.io/library/alpine:3.14. After executing the command sudo systemctl restart docker to restart the Docker service, it appears that the issue related to loading metadata for the alpine:3.14 image and making requests to the Docker registry (registry-1.docker.io) has been resolved. make gen-api-docs command is now working fine.

Huh! Weird!

Glad you got it sorted!

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.

One small things to fix, otherwise, this looks good to go.

pkg/fleetautoscalers/controller.go Show resolved Hide resolved
@Kalaiselvi84
Copy link
Contributor Author

qq, I haven't added feature shortcodes in any of the files. Does everything look good?

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.

Good point on the feature shortcodes 👍🏻

site/content/en/docs/Guides/feature-stages.md Show resolved Hide resolved
@@ -37,8 +37,6 @@ spec:
# maximum fleet size that can be set by this FleetAutoscaler
# required
maxReplicas: 20
# [Stage:Beta]
Copy link
Member

Choose a reason for hiding this comment

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

This update should also be feature shortcoded please.

Copy link
Contributor Author

@Kalaiselvi84 Kalaiselvi84 Jul 7, 2023

Choose a reason for hiding this comment

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

I have a little doubt about this update. Is this correct?

{{% feature expiryVersion="1.34.0" %}}
# [Stage:Beta]
# [FeatureFlag:CustomFasSyncInterval]
{{% /feature %}}

Copy link
Member

Choose a reason for hiding this comment

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

Thaaat might work? But I'm betting it won't.

If you run make site-server you can preview your changes locally and see what works and what doesn't.

Copy link
Contributor Author

@Kalaiselvi84 Kalaiselvi84 Jul 7, 2023

Choose a reason for hiding this comment

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

Is the behavior shown in the screenshot at https://screenshot.googleplex.com/5nzmXVU6eacV8Kv expected?

There was an issue with make site-server command. This problem has been resolved by adding git config --global --add safe.directory /go/src/agones.dev/agones into the existing code.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, that doesn't look right (it should be gone), you can also see the preview here: https://1b8d8e1-dot-preview-dot-agones-images.appspot.com/site/docs/reference/fleetautoscaler/

I expect you'll need to feature code the entire block.

Copy link
Contributor Author

@Kalaiselvi84 Kalaiselvi84 Jul 7, 2023

Choose a reason for hiding this comment

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

I haven't observed any variation in my local environment, whether I utilize feature shortcodes or not. Even if I remove the entire block, I still encounter these lines:

# [Stage:Beta]
# [FeatureFlag:CustomFasSyncInterval]

Could you please confirm if it would be harmful if I remove these lines without having the corresponding feature shortcodes?

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 I wasn't clear - much like we do with markdown tables, we would have to duplicate the whole

```yaml
....
...

block.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried this too and it was not working in my local 😞 There might be some issue with my local env. committed the changes..

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: 6344132a-c4e0-4ddb-8790-7862c4195bd3

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/googleforgames/agones.git pull/3235/head:pr_3235 && git checkout pr_3235
  • helm install agones ./install/helm/agones --namespace agones-system --set agones.image.registry=us-docker.pkg.dev/agones-images/ci --set agones.image.tag=1.33.0-dev-1b8d8e1-amd64

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: 7e368d98-5642-4d0a-99cb-8d45883276a1

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/googleforgames/agones.git pull/3235/head:pr_3235 && git checkout pr_3235
  • helm install agones ./install/helm/agones --namespace agones-system --set agones.image.registry=us-docker.pkg.dev/agones-images/ci --set agones.image.tag=1.33.0-dev-4e45825-amd64

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: 90795fd9-a13e-46de-ab37-67da5d2ab50d

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/googleforgames/agones.git pull/3235/head:pr_3235 && git checkout pr_3235
  • helm install agones ./install/helm/agones --namespace agones-system --set agones.image.registry=us-docker.pkg.dev/agones-images/ci --set agones.image.tag=1.34.0-dev-cc537c5-amd64

@markmandel
Copy link
Member

@google-oss-prow google-oss-prow bot added the lgtm label Jul 10, 2023
@markmandel markmandel merged commit 635573e into googleforgames:main Jul 10, 2023
@google-oss-prow
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Kalaiselvi84, markmandel

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@Kalaiselvi84 Kalaiselvi84 deleted the issues/288583663 branch March 15, 2024 01:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Graduate CustomFasSyncInterval to Stable
3 participants