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

Used array of FStringFormatArg to process FString::Format to fix erro… #2170

Merged
merged 7 commits into from
Jul 28, 2021
Merged

Conversation

WilSimpson
Copy link
Contributor

…rs in BuildAgonesRequest

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 #2169

Special notes for your reviewer:

@google-cla
Copy link

google-cla bot commented Jul 3, 2021

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@WilSimpson
Copy link
Contributor Author

@googlebot I signed it!

@google-cla
Copy link

google-cla bot commented Jul 3, 2021

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

1 similar comment
@google-cla
Copy link

google-cla bot commented Jul 3, 2021

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

@WilSimpson
Copy link
Contributor Author

@googlebot I fixed it.

@google-cla
Copy link

google-cla bot commented Jul 3, 2021

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

@WilSimpson
Copy link
Contributor Author

I'm unable to update for my other personal email address used [email protected] since it has been used by someone else for their Google account so I cannot verify that one. I have however committed with my GitHub email so it all matches for one. /assign @pooneh-m

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: ec2bf119-ed2c-46f1-8e64-66883d688646

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/2170/head:pr_2170 && git checkout pr_2170
  • helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.16.0-ea27510

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: 7510b8dd-a61e-492d-b016-d0c4aa9f36e4

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/2170/head:pr_2170 && git checkout pr_2170
  • helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.16.0-badbde4

@WilSimpson
Copy link
Contributor Author

/assign @pooneh-m

@roberthbailey
Copy link
Member

@WilSimpson - Thanks for the bug fix! Can you please sign the CLA?

@roberthbailey
Copy link
Member

This look reasonable to me, but I'd like to get a quick review from @domgreen who has been helping maintain our unreal code.

/assign @domgreen

@google-oss-robot
Copy link

@roberthbailey: GitHub didn't allow me to assign the following users: domgreen.

Note that only googleforgames members, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

This look reasonable to me, but I'd like to get a quick review from @domgreen who has been helping maintain our unreal code.

/assign @domgreen

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@WilSimpson
Copy link
Contributor Author

@WilSimpson - Thanks for the bug fix! Can you please sign the CLA?

Long story short I can't for this fork due to issues with the original committing email. Worst case I can create a new fork and commit with a different email.

@roberthbailey
Copy link
Member

Worst case I can create a new fork and commit with a different email.

We can't merge a change without the CLA being signed, so if that is the only way to appease the CLA, then it sounds like the best path forward.

@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

@markmandel
Copy link
Member

Long story short I can't for this fork due to issues with the original committing email. Worst case I can create a new fork and commit with a different email.

If you rebase this PR down to a single commit authored with your @gmail.com address, and force push over the top, then the CLA bot should accept it without issue.

Also calling @highlyunavailable who has been doing lots of Unreal stuff as of late.

@markmandel
Copy link
Member

Just a heads up - Release Candidate is next Tuesday, so if you would like this fix in the next release, that'll be the target to aim for 👍🏻

@roberthbailey roberthbailey added the feature-freeze-do-not-merge Only eligible to be merged once we are out of feature freeze (next full release) label Jul 13, 2021
@WilSimpson WilSimpson closed this Jul 14, 2021
@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: 145b8115-4483-4930-afe6-f3b6eec3093a

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/2170/head:pr_2170 && git checkout pr_2170
  • helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.17.0-8ead913

@google-cla
Copy link

google-cla bot commented Jul 28, 2021

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: 3e5a88f3-d944-48db-8871-28927b4d2de2

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

@google-cla
Copy link

google-cla bot commented Jul 28, 2021

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@WilSimpson
Copy link
Contributor Author

I have updated the changes as requested by @highlyunavailable. Everything runs smoothly on UE5ea2. I did not test in 4.26, however I do not believe these changes should effect 4.26 as it follows their guidelines as well.

There were not issues with passing a single element, however I converted them all for completeness and to avoid future errors. Since there was so much repetitive code, I included a few static method helper functions. I believe the few cycles added by making the additional private static methods out weigh the readability for the repetitiveness. Let me know if you disagree.

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: 09cec38f-51dc-442d-be65-3739e232660b

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/2170/head:pr_2170 && git checkout pr_2170
  • helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.17.0-d1a81d9

Copy link
Contributor

@highlyunavailable highlyunavailable left a comment

Choose a reason for hiding this comment

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

Looks fine to me. There may be some formatting nits but I can confirm it builds fine on 4.26.2 as well.

@google-oss-robot
Copy link

@highlyunavailable: changing LGTM is restricted to collaborators

In response to this:

Looks fine to me. There may be some formatting nits but I can confirm it builds fine on 4.26.2 as well.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

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.

Yay! Looks like we have a winner. Approving and merging!

@google-oss-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: highlyunavailable, markmandel, WilSimpson

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

@google-oss-robot
Copy link

New changes are detected. LGTM label has been removed.

@google-cla
Copy link

google-cla bot commented Jul 28, 2021

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: 5e34d22e-8d85-4808-b6a0-c62722b3e732

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/2170/head:pr_2170 && git checkout pr_2170
  • helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.17.0-f2ed4a6

@google-cla
Copy link

google-cla bot commented Jul 28, 2021

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: 8ceef899-bd68-403a-b5a2-74bea5527eba

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/2170/head:pr_2170 && git checkout pr_2170
  • helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.17.0-f4967be

@markmandel markmandel merged commit 8ca4929 into googleforgames:main Jul 28, 2021
@markmandel markmandel added this to the 1.17.0 milestone Jul 28, 2021
@markmandel markmandel added area/user-experience Pertaining to developers trying to use Agones, e.g. SDK, installation, etc kind/bug These are bugs. labels Jul 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved area/user-experience Pertaining to developers trying to use Agones, e.g. SDK, installation, etc cla: yes kind/bug These are bugs. size/L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Errors in Unreal Engine SDK BuildAgonesRequest
8 participants