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

v4 Updates #433

Merged
merged 16 commits into from
Nov 15, 2023
Merged

v4 Updates #433

merged 16 commits into from
Nov 15, 2023

Conversation

AndrewKahr
Copy link
Member

@AndrewKahr AndrewKahr commented Oct 27, 2023

Changes

  • Add containerRegistryRepository, containerRegistryImageVersion, dockerCpuLimit, dockerMemoryLimit, and dockerIsolationMode information
  • Change windows-2019 to windows-2022 in getting started
  • Bumped out-of-date action versions throughout example workflows
  • Update out-of-date parameter on Android deployment workflow example

Related PRs

Related Issues

Checklist

  • Read the contribution guide and accept the
    code of conduct
  • Readme (updated or not needed)
  • Tests (added, updated or not needed)

@github-actions
Copy link

Cat Gif

@github-actions
Copy link

github-actions bot commented Oct 27, 2023

Visit the preview URL for this PR (updated for commit 8ebb6b5):

https://game-ci-5559f--pr433-windows-updates-e4duf1pm.web.app

(expires Wed, 22 Nov 2023 04:15:25 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: 1f0574f15f83e11bfc148eae8646486a6d0e078b

@AndrewKahr AndrewKahr changed the title Add new parameters for cpu and memory limits Windows Upgrades Oct 28, 2023
docs/03-github/04-builder.mdx Outdated Show resolved Hide resolved
docs/03-github/04-builder.mdx Outdated Show resolved Hide resolved
…ated versions for other actions in example workflows
…ion mode to test runner. Fix various image versions being out of date. Improve isolation mode description
@GabLeRoux GabLeRoux self-requested a review November 2, 2023 02:23

#### dockerMemoryLimit

Amount of memory to assign the docker container. Defaults to 95% of total system memory rounded down
Copy link
Member

Choose a reason for hiding this comment

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

nitpick: simply wondering if we should have consistency with "Windows docker container" vs "docker container" in here.


#### dockerIsolationMode

Isolation mode to use for the docker container. Can be one of process, hyperv, or default. Default
Copy link
Member

Choose a reason for hiding this comment

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

Same nitpick about consistency for nitpick: "Windows docker container" vs "docker container"

#### dockerIsolationMode

Isolation mode to use for the docker container. Can be one of process, hyperv, or default. Default
will pick the default mode as described by Microsoft where server versions use process and desktop
Copy link
Member

Choose a reason for hiding this comment

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

I find the repetition of default in here a bit confusing, maybe we should replace with recommended instead? really not sure about this one

Suggested change
will pick the default mode as described by Microsoft where server versions use process and desktop
will pick the recommended mode as described by Microsoft where server versions use process and desktop

versions use hyperv. Only applicable on Windows

_**required:** `false`_ _**default:** `"default"`_

Copy link
Member

Choose a reason for hiding this comment

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

Here's a suggestion combining some the above ideas. I used a list for m and g units for dockerMemoryLimit. Using e.g. instead of ie for examples. I'm definitely ok to merge as it is right now by the way.


dockerCpuLimit

Number of CPU cores to assign to the Docker container. Defaults to all available cores when no value is specified. Can accept fractional values, e.g., 0.5 for half of a CPU core's execution time.

required: false default: ""

dockerMemoryLimit

Amount of memory to assign to the Docker container. Defaults to 95% of total system memory rounded down to the nearest megabyte on Linux and 80% on Windows. If the platform is unrecognized, it defaults to 75% of total system memory. To manually specify a value, use the format <number><unit>. The units can be:

  • m: Megabytes (e.g., 512m = 512 megabytes)
  • g: Gigabytes (e.g., 4g = 4 gigabytes)

required: false default: ""

dockerIsolationMode

Isolation mode to use for the Docker container when running on Windows. Can be one of:

  • process
  • hyperv
  • default: This mode will choose the recommended mode as described by Microsoft. Server versions will use process, while desktop versions will use hyperv.

required: false default: "default"

Copy link
Member Author

Choose a reason for hiding this comment

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

I like this a lot better, will be using this strategy. Excellent suggestion!

@AndrewKahr AndrewKahr changed the title Windows Upgrades v4 Updates Nov 5, 2023
@davidmfinol
Copy link
Member

I see you replaced androidAppBundle with androidExportType same time as I did! 😆
I can just close the other PR I raised.

@AndrewKahr
Copy link
Member Author

Haha yea I realized a bit too late. Feel free to merge yours so that it hits v3 while we wait for v4 changes to wrap up which might take a bit. I backported the change to the v3 docs in this PR so shouldn't be an issue

@AndrewKahr AndrewKahr merged commit 4747e1c into main Nov 15, 2023
6 checks passed
@AndrewKahr AndrewKahr deleted the windows-updates branch November 15, 2023 15:36
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.

4 participants