ci: Enable nightly build for Windows MSI and integrate atom action into 1 click automation #695
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Vishwas Siravara <[email protected]>
Signed-off-by: Vishwas Siravara <[email protected]>
Signed-off-by: Vishwas Siravara <[email protected]>
Signed-off-by: Vishwas Siravara <[email protected]>
Issue #, if available: *Description of changes:* - introduces aliases for nerdctl commands. Eg `nerdctl run` is equivalent to `nerdctl container run`. cli commands are restructured based on https://www.docker.com/blog/whats-new-in-docker-1-13/ , `nerdctl container run` is the recommended way of using the cli. This also helps in handling options for commands and avoids duplication for having to handle the same options for equivalent commands such as `run` and `container run`. - introduces command handlers. command handlers are functions that handle arguments of a specific command. For example `cpHandler`, handles `nerdctl cp` command. It has a one to one mapping with nerdctl commands that need command handling. - introduces argHandlers, which handle particular options in nerdctl commands. These are not tied to a specific command. For instance `handleFilePath` function can handle `-f/--file` option in `nerdctl build`. It can also handle `--label-file` in `nerdctl run` command. The mapping is defined by `argHandlerMap`. *Testing done:* Yes, unit tests added for cmdhandler and argHandlers. all container e2e tests and passed. ----- #### Notes There were several options considered for translating windows filepaths to wsl filepaths for nerdctl commands. * add logic [handleEnv](https://github.com/runfinch/finch/blob/main/cmd/finch/nerdctl.go#L229) function to handle options. This approach had several disadvantages * options require different handling depending on the command. For instance -f/--file option in build command has different semantics to -f in rm command. * Not extensible if new options are added to nerdctl command. * search and replace. match every argument in a nerdctl to a windows filepath. If a match is found then replace it with it's equivalent wsl filepath. This approach also has a couple of problems. * There is no perfect regex to match windows paths in nerdctl commands. Windows paths can have spaces and can also be relative paths and when used in conjunction with nerdctl commands there is ambiguity. Eg nerdctl cp C:\\Test USER conname:<containerpath>. Do we match C:\\Test USER or C:\\Test USER conname since both are valid windows paths. * Matching every argument adds to the computation cost of parsing nerdctl commands. Therefore the choice was made to have a mapping between commands and options with the path translation logic which can guarantee correctness in handling nerdctl arguments(if implemented correctly :) ) and is extensible. For macOS, the handers are no-op. We can refactor [handling logic ](https://github.com/runfinch/finch/blob/main/cmd/finch/nerdctl.go#L229) in a later PR. ------- - [X] I've reviewed the guidance in CONTRIBUTING.md #### License Acceptance By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license. --------- Signed-off-by: Vishwas Siravara <[email protected]> Co-authored-by: Vishwas Siravara <[email protected]> Signed-off-by: Vishwas Siravara <[email protected]>
Signed-off-by: Vishwas Siravara <[email protected]>
Issue #, if available: *Description of changes:* keep windev submodules up to date to unblock CI changes *Testing done:* - [x] I've reviewed the guidance in CONTRIBUTING.md #### License Acceptance By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license. Signed-off-by: Gavin Inglis <[email protected]> Signed-off-by: Vishwas Siravara <[email protected]>
Signed-off-by: Vishwas Siravara <[email protected]> Signed-off-by: Vishwas Siravara <[email protected]>
~~**Do not merge**~~ Issue #, if available: *Description of changes:* Fix all lint errors in windows. *Testing done:* Yes. Unit test and golangci-lint. - [X] I've reviewed the guidance in CONTRIBUTING.md #### License Acceptance By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license. --------- Signed-off-by: Vishwas Siravara <[email protected]> Co-authored-by: Vishwas Siravara <[email protected]> Signed-off-by: Vishwas Siravara <[email protected]>
Issue #, if available: Mismatch in rootfs between finch and finch-core on respective windev branches - both in the version and in the file referenced. finch-core is correctly providing full .tar.gz extension and latest rootfs. Evident in jobs: * https://github.com/runfinch/finch/actions/runs/6386280972/job/17332665289?pr=584 * https://github.com/runfinch/finch/actions/runs/6386379434/job/17332965393?pr=584 *Description of changes:* *Testing done:* Validated on windows dev instance. - [x] I've reviewed the guidance in CONTRIBUTING.md #### License Acceptance By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license. Signed-off-by: Gavin Inglis <[email protected]> Signed-off-by: Vishwas Siravara <[email protected]>
Issue #, if available: *Description of changes:* Update project submodules for Windows development. *Testing done:* - [x] I've reviewed the guidance in CONTRIBUTING.md #### License Acceptance By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license. Signed-off-by: Gavin Inglis <[email protected]> Signed-off-by: Vishwas Siravara <[email protected]>
Issue #, if available: Add GOOS to golangci-lint. Currently the golangci-lint runs on ubuntu. *Description of changes:* - Change target lint in makefile to run linter for both windows and macOS. - Update ci.yaml to lint on macOS and windows github hosted runners. *Testing done:* Makefile changes validated. - [X] I've reviewed the guidance in CONTRIBUTING.md #### License Acceptance By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license. --------- Signed-off-by: Vishwas Siravara <[email protected]> Co-authored-by: Vishwas Siravara <[email protected]> Signed-off-by: Vishwas Siravara <[email protected]>
Issue #, if available: *Description of changes:* - Adds persistent disk support to Windows - This took more work than anticipated, in order to deal with non-Admin users - `pkg/disk/dpgo/` is brand new code, and should be a focus of the review - `pkg/winutil/run_windows.go` is also new and requires careful review. This is what allows Finch to run as the regular user, except for when it needs Admin access to call `diskpart` (to create the persistent disk) - Fix paths in `nerdctl_config_applier` to make the post-boot/init shelling work - Added winres to allow the finch.exe to have metadata attached to it. This is WIP, need final icons and descriptions etc. - Large (in terms of lines changed) refactor of `pkg/path/finch.go`, but it should have no impact on functionality (needs careful review) - Fixed the Makefile's `clean` target for Windows - Most of the other changes are just noise from refactoring (like, literally renaming things). Nothing major, but take a look if possible. Sorry the diff is so large *Testing done:* - [x] I've reviewed the guidance in CONTRIBUTING.md By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license. --------- Signed-off-by: Vishwas Siravara <[email protected]> Signed-off-by: Justin Alvarez <[email protected]> Signed-off-by: Vishwas Siravara <[email protected]> Co-authored-by: Vishwas Siravara <[email protected]> Co-authored-by: Vishwas Siravara <[email protected]> Signed-off-by: Vishwas Siravara <[email protected]>
Issue #, if available: *Description of changes:* Update run tests related to cgroup v2 resource flag tests for windows. In wsl2 cgroup v2 is mounted at /sys/fs/cgroup/unified but containerd expects it to be mounted at /sys/fs/cgroup https://github.com/containerd/cgroups/blob/cc78c6c1e32dc5bde018d92999910fdace3cfa27/utils.go#L36. Enabling cgroup v2 requires extra tweaks in wsl 2 based on https://stackoverflow.com/questions/73021599/how-to-enable-cgroup-v2-in-wsl2 *Testing done:* Yes. - [X] I've reviewed the guidance in CONTRIBUTING.md #### License Acceptance By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license. --------- Signed-off-by: Vishwas Siravara <[email protected]> Co-authored-by: Vishwas Siravara <[email protected]> Signed-off-by: Vishwas Siravara <[email protected]>
Issue #, if available: *Description of changes:* Fix filepath in `nerdctl_config_applier_test.go` *Testing done:* Yes, locally. - [X] I've reviewed the guidance in CONTRIBUTING.md #### License Acceptance By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license. Signed-off-by: Vishwas Siravara <[email protected]> Co-authored-by: Vishwas Siravara <[email protected]> Signed-off-by: Vishwas Siravara <[email protected]>
Issue #, if available: *Description of changes:* - fix unit tests for darwin - multi line comment for golinter - remove duplicate test in config_test.go *Testing done:* Yes, locally. - [X] I've reviewed the guidance in CONTRIBUTING.md #### License Acceptance By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license. Signed-off-by: Vishwas Siravara <[email protected]>
Issue #, if available: *Description of changes:* `go mod tidy` *Testing done:* - [x] I've reviewed the guidance in CONTRIBUTING.md By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license. Signed-off-by: Gavin Inglis <[email protected]> Signed-off-by: Vishwas Siravara <[email protected]>
Issue #, if available: *Description of changes:* Fix finch config test *Testing done:* Yes, locally. - [X] I've reviewed the guidance in CONTRIBUTING.md #### License Acceptance By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license. Signed-off-by: Vishwas Siravara <[email protected]> Co-authored-by: Vishwas Siravara <[email protected]> Signed-off-by: Vishwas Siravara <[email protected]>
Issue #, if available: *Description of changes:* - use `cmd /c ver` to get windows version. - fix regex for windows path *Testing done:* Yes, support bundle generated locally. - [X] I've reviewed the guidance in CONTRIBUTING.md #### License Acceptance By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license. Signed-off-by: Vishwas Siravara <[email protected]> Co-authored-by: Vishwas Siravara <[email protected]> Signed-off-by: Vishwas Siravara <[email protected]>
Issue #, if available: *Description of changes:* Update submodules *Testing done:* - [x] I've reviewed the guidance in CONTRIBUTING.md #### License Acceptance By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license. Signed-off-by: Gavin Inglis <[email protected]> Signed-off-by: Vishwas Siravara <[email protected]>
Issue #, if available:
*Description of changes:*
- Pinned all actions to commit SHAs
- Just used the latest available version
- Dependabot should actually update the version comment too, thanks to
[this
feature](dependabot/dependabot-core#4691),
which is pretty neat
- `Homebrew/actions/setup-homebrew` isn't really "vended" and the
recommendation is to just use the latest master commit of the repo,
which is why the version name just says "master"
- `WyriHaximus/github-action-get-previous-tag` hasn't been updated in a
while and the functionality doesn't seem that hard to replicate on our
own. Might be good to remove it in the future
*Testing done:*
- [x] I've reviewed the guidance in CONTRIBUTING.md
#### License Acceptance
By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 license.
Signed-off-by: Justin Alvarez <[email protected]>
Signed-off-by: Vishwas Siravara <[email protected]>
Issue #, if available: *Description of changes:* - change path -> filepath - close files after opening, since can't delete on windows without close. *Testing done:* Yes, all support bundle e2e tests. - [X] I've reviewed the guidance in CONTRIBUTING.md #### License Acceptance By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license. Signed-off-by: Vishwas Siravara <[email protected]> Co-authored-by: Vishwas Siravara <[email protected]> Signed-off-by: Vishwas Siravara <[email protected]>
Issue #, if available: *Description of changes:* upstream dependency go-winres uses `git tag` to embed versioning in Windows binary. However, if the repo from which finch is being built has no tags, build will fail. Add note on how to fetch upstream finch tags. See tc-hib/go-winres#16 *Testing done:* - [x] I've reviewed the guidance in CONTRIBUTING.md #### License Acceptance By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license. Signed-off-by: Gavin Inglis <[email protected]> Signed-off-by: Vishwas Siravara <[email protected]>
Issue #, if available: *Description of changes:* Address all linter findings, including ignoring gosec warnings about auditing the use of unsafe pointers in pkg/winutil/run_windows.go https://securego.io/docs/rules/g103.html *Testing done:* `make lint` - [x] I've reviewed the guidance in CONTRIBUTING.md #### License Acceptance By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license. Signed-off-by: Gavin Inglis <[email protected]> Signed-off-by: Vishwas Siravara <[email protected]>
Issue #, if available: *Description of changes:* Bump common-tests to v0.7.7 *Testing done:* Yes. See https://github.com/runfinch/finch/actions/runs/6434443967 - [X] I've reviewed the guidance in CONTRIBUTING.md By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license. Signed-off-by: Vishwas Siravara <[email protected]> Co-authored-by: Vishwas Siravara <[email protected]> Signed-off-by: Vishwas Siravara <[email protected]>
Issue #, if available:
Mitigate excessive permissions on files and directory creation.
*Description of changes:*
- sets restrictive permissions on files to 0o600 and 0o700 for creating
directories.
*Testing done:*
Yes, unit tests and sanity checks.
```
grep -RE '[0-9]o[0-9]{3}' --include='*.go' --exclude='*_test.go'
./benchmark/suite.go: `, alpineImage)), 0o600)
./deps/finch-core/src/lima/pkg/yqutil/yqutil.go: err = os.WriteFile(tmpYAMLPath, content, 0o600)
./deps/finch-core/src/lima/pkg/editutil/editutil.go: 0o600); err != nil {
./pkg/config/config.go: if err := afero.WriteFile(fs, path, cfgBuf, 0o600); err != nil {
./pkg/config/config.go: if err := fs.Mkdir(path, 0o700); err != nil {
./pkg/config/nerdctl_config_applier.go: if err := afero.WriteFile(fs, profileFilePath, []byte(profBufWithCmd), 0o600); err != nil {
./pkg/config/nerdctl_config_applier.go: if err := fs.MkdirAll(path.Dir(cfgPath), 0o700); err != nil {
./pkg/config/nerdctl_config_applier.go: if err := afero.WriteFile(fs, cfgPath, []byte{}, 0o600); err != nil {
./pkg/config/nerdctl_config_applier.go: if err := afero.WriteFile(fs, cfgPath, updatedCfg, 0o600); err != nil {
./pkg/config/lima_config_applier.go: if err := afero.WriteFile(lca.fs, lca.limaConfigPath, []byte(""), 0o600); err != nil {
./pkg/config/lima_config_applier.go: if err := afero.WriteFile(lca.fs, lca.limaConfigPath, limaCfgBytes, 0o600); err != nil {
./pkg/dependency/vmnet/update_override_lima_config_unix.go: f, err := overConf.fs.OpenFile(filePath, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0o644)
./pkg/dependency/credhelper/cred_helper_binary.go: file, err := bin.fs.OpenFile(cfgPath, os.O_RDWR|os.O_CREATE|os.O_TRUNC, 0o600)
./pkg/dependency/credhelper/cred_helper_binary.go: err = bin.fs.Chmod(bin.fullInstallPath(), 0o700)
./pkg/disk/disk_unix.go: if err := m.fs.MkdirAll(disksDir, 0o700); err != nil {
./pkg/disk/disk_windows.go: if err := m.fs.MkdirAll(disksDir, 0o700); err != nil {
./pkg/disk/disk_windows.go: if err := m.fs.MkdirAll(disksDir, 0o700); err != nil {
```
- [X] I've reviewed the guidance in CONTRIBUTING.md
#### License Acceptance
By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 license.
Signed-off-by: Vishwas Siravara <[email protected]>
Co-authored-by: Vishwas Siravara <[email protected]>
Signed-off-by: Vishwas Siravara <[email protected]>
Issue #, if available:
*Description of changes:*
- Refactor credhelper and remove hard-coded paths.
*Testing done:*
Yes, can push to ECR
```
cat .\config.json
{"credsStore":"ecr-login"}
PS C:\Users\Administrator> finch build -t test-ecr-from-local .
PS C:\Users\Administrator> finch tag test-ecr-from-local:latest 065363855432.dkr.ecr.us-east-2.amazonaws.com/test-ecr-from-local:latest
PS C:\Users\Administrator> finch tag test-ecr-from-local:latest 065363855432.dkr.ecr.us-east-2.amazonaws.com/test-ecr-from-local:latest^C
PS C:\Users\Administrator> finch push 065363855432.dkr.ecr.us-east-2.amazonaws.com/test-ecr-from-local:latest
INFO[0000] pushing as a reduced-platform image (application/vnd.docker.distribution.manifest.v2+json, sha256:317cda6f2c180388322d9134cf8976aac777a5d6cf1a56161ac22e996a1df051)
manifest-sha256:317cda6f2c180388322d9134cf8976aac777a5d6cf1a56161ac22e996a1df051: done |++++++++++++++++++++++++++++++++++++++|
config-sha256:1cad2846fc76fdde7217fb06db38fbcdccaed71baccc30a7e8ec70f095ea3d11: done |++++++++++++++++++++++++++++++++++++++|
elapsed: 1.1 s total: 1.1 Ki (1001.0 B/s)
```
- [X] I've reviewed the guidance in CONTRIBUTING.md
#### License Acceptance
By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 license.
Signed-off-by: Vishwas Siravara <[email protected]>
Co-authored-by: Vishwas Siravara <[email protected]>
Signed-off-by: Vishwas Siravara <[email protected]>
Issue #, if available:
*Description of changes:*
disks in the list format in lima override.yaml were not being
unmarshalled properly, causing a disk with the name "" to be mixed with
finch lima yaml, resulting in startup failure. This only occurs on inits
of a vm other than the very first init. init a vm the first time,
_output/lima/data/_config/override.yaml:
```yaml
additionalDisks:
- name: finch
```
stop and remove the vm, then re-init:
```yaml
additionalDisks:
- name: ""
- name: finch
```
to fix, copy upstream logic from lima to use a custom unmarshaler for
the Disk type.
*Testing done:*
`make test-unit`
manual testing
- [x] I've reviewed the guidance in CONTRIBUTING.md
#### License Acceptance
By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 license.
Signed-off-by: Gavin Inglis <[email protected]>
Signed-off-by: Vishwas Siravara <[email protected]>
Issue #, if available: *Description of changes:* - fix path for deleting persistent disk for windows for cleanup in e2e tests - fix path for finch config e2e vm test *Testing done:* Yes, locally - [X] I've reviewed the guidance in CONTRIBUTING.md #### License Acceptance By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license. --------- Signed-off-by: Vishwas Siravara <[email protected]> Co-authored-by: Vishwas Siravara <[email protected]> Signed-off-by: Vishwas Siravara <[email protected]>
Issue #, if available: *Description of changes:* - update finch-core submod - update rootfs archive in Makefile *Testing done:* Yes, cosign e2e test locally. - [X] I've reviewed the guidance in CONTRIBUTING.md #### License Acceptance By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license. Signed-off-by: Vishwas Siravara <[email protected]> Co-authored-by: Vishwas Siravara <[email protected]> Signed-off-by: Vishwas Siravara <[email protected]>
Issue #, if available: *Description of changes:* - Update README to add instructions for installing and running finch on windows. *Testing done:* - [X] I've reviewed the guidance in CONTRIBUTING.md #### License Acceptance By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license. --------- Signed-off-by: Vishwas Siravara <[email protected]>
Issue #, if available: *Description of changes:* Use the `always()` built in status check function to always run cleanup steps, even if a job is cancelled: https://docs.github.com/en/actions/learn-github-actions/expressions#status-check-functions *Testing done:* refactor - [x] I've reviewed the guidance in CONTRIBUTING.md #### License Acceptance By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license. Signed-off-by: Gavin Inglis <[email protected]> Co-authored-by: Kevin Li <[email protected]> Signed-off-by: Vishwas Siravara <[email protected]>
Issue #, if available: *Description of changes:* - disable MD24 lint for duplicate non sibling headers. *Testing done:* Yes, local. - [X] I've reviewed the guidance in CONTRIBUTING.md #### License Acceptance By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license. Signed-off-by: Vishwas Siravara <[email protected]>
Signed-off-by: Vishwas Siravara <[email protected]>
Signed-off-by: Vishwas Siravara <[email protected]>
Signed-off-by: Vishwas Siravara <[email protected]>
Signed-off-by: Justin Alvarez <[email protected]>
Signed-off-by: Justin Alvarez <[email protected]>
…-click automation Signed-off-by: Kevin Li <[email protected]>
vsiravar
reviewed
Nov 16, 2023
vsiravar
reviewed
Nov 16, 2023
| uses: ./.github/workflows/upload-msi-to-release.yaml | ||
| secrets: inherit | ||
| with: | ||
| ref_name: ${{ needs.get-latest-tag.outputs.tag }} |
Signed-off-by: Justin Alvarez <[email protected]>
pendo324
changed the title
pendo324
changed the title
KevinLiAWS
commented
Jan 11, 2024
| ref_name: ${{ needs.get-latest-tag.outputs.tag }} | ||
|
|
||
| upload-msi-to-release: | ||
| needs: |
There was a problem hiding this comment.
Dense for this line is wrong
There was a problem hiding this comment.
Thanks, seems like I messed up some of the merge conflicts. Will push fixes after tests run.
KevinLiAWS
commented
Jan 11, 2024
.github/workflows/lint-pr-title.yaml
Outdated
| @@ -14,7 +14,11 @@ jobs: | |||
| name: conventional-commit | |||
| runs-on: ubuntu-latest | |||
| steps: | |||
| <<<<<<< HEAD | |||
There was a problem hiding this comment.
Seems have unresolved conflict
There was a problem hiding this comment.
Made the change locally but somehow didn't commit it before merging the merge commit :(
vsiravar
reviewed
Jan 11, 2024
…eeded Signed-off-by: Justin Alvarez <[email protected]>
Signed-off-by: Justin Alvarez <[email protected]>
|
All comments should be resolved now @KevinLiAWS @vsiravar |
pendo324
approved these changes
Jan 19, 2024
|
Failing tests are unrelated, overriding and merging. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Enable nightly build for Windows MSI and integrate atom action into 1 click automation
Description of changes:
Added the scheduled time to Windows build and test action.
Testing done:
TODO
License Acceptance
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.