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

Do not export GOROOT for Go versions >= 1.9 #175

Merged
merged 6 commits into from Mar 17, 2022

Conversation

mmlb
Copy link
Contributor

@mmlb mmlb commented Dec 14, 2021

Description:

Do not export GOROOT. This has not been necessary since Go 1.9 at least (although clunky) but definitely not since Go 1.10. This is cargo culting code that is more than 2 years out of date and runs into issues when multiple go versions are used in an action run.

Related issue:

#107

Check list:

  • Mark if documentation changes are required.
  • Mark if tests were added or updated to cover the changes.

@mmlb
Copy link
Contributor Author

@mmlb mmlb commented Dec 14, 2021

Opened this up as a draft to get CI running as I'm a super n00b at ts/js. I wanted to test the current behavior (adding GOROOT) before removing it and updating the test.

@mmlb mmlb force-pushed the do-not-export-GOROOT branch from 223510d to 9735516 Compare Dec 14, 2021
@mmlb
Copy link
Contributor Author

@mmlb mmlb commented Dec 14, 2021

The actual fix commit I'll add afterwards is:

commit 607cfc72940f41ed709d01f36a75387ed7d150dd
Author: Manuel Mendez <mmendez534@gmail.com>
Date:   Tue Dec 14 13:24:18 2021 -0500

    Do not export GOROOT
    
    This has not been necessary since [Go 1.9](https://go.dev/doc/go1.9#goroot) at least (although clunky) but definitely not since [Go 1.10](https://go.dev/doc/go1.10#goroot).
    This is cargo culting code that is more than 2 years out of date and runs into issues when multiple go versions are used in an action run.
    
    Signed-off-by: Manuel Mendez <mmendez534@gmail.com>

diff --git a/__tests__/setup-go.test.ts b/__tests__/setup-go.test.ts
index a001839..9368804 100644
--- a/__tests__/setup-go.test.ts
+++ b/__tests__/setup-go.test.ts
@@ -243,7 +243,7 @@ describe('setup-go', () => {
     });
 
     await main.run();
-    expect(vars).toBe({'GOROOT': 'foo'});
+    expect(vars).toBe({});
   });
 
   it('finds a version of go already in the cache', async () => {
diff --git a/src/main.ts b/src/main.ts
index 2d90b2f..29ae2b6 100644
--- a/src/main.ts
+++ b/src/main.ts
@@ -26,7 +26,6 @@ export async function run() {
 
       const installDir = await installer.getGo(versionSpec, stable, auth);
 
-      core.exportVariable('GOROOT', installDir);
       core.addPath(path.join(installDir, 'bin'));
       core.info('Added go to the path');

@mmlb mmlb changed the title Add test for export of GOROOT to env var Do not export GOROOT Dec 14, 2021
@mmlb mmlb marked this pull request as ready for review Dec 14, 2021
@mmlb mmlb requested a review from Dec 14, 2021
@mmlb
Copy link
Contributor Author

@mmlb mmlb commented Jan 19, 2022

Gentle ping, @spark

@mmlb
Copy link
Contributor Author

@mmlb mmlb commented Jan 19, 2022

Pinging @bryanmacfarlane and @joshmgross because it seems the spark I pinged isn't the same spark as in reviewers.

mmlb added a commit to tinkerbell/tink that referenced this issue Jan 19, 2022
## Description

Drop go from shell.nix (for now) and use the system's go binary.

## Why is this needed

CI is currently breaking because of conflicting go versions.
The better fix would be cutting over fully to nix in CI or reverting this commit once actions/setup-go#175 is merged and released.

## How Has This Been Tested?

CI

## How are existing users impacted? What migration steps/scripts do we need?

CI isn't broken and PRs can be merged.
@joshmgross joshmgross requested a review from Jan 19, 2022
@mmlb mmlb force-pushed the do-not-export-GOROOT branch from 607cfc7 to 3730c7e Compare Mar 7, 2022
@mmlb
Copy link
Contributor Author

@mmlb mmlb commented Mar 7, 2022

Hi @joshmgross I've updated to address a merge conflict. It'd be really nice if someone could take a look here.

@joshmgross
Copy link
Member

@joshmgross joshmgross commented Mar 8, 2022

👋 @mmlb could you take a look at the failing CI?

It looks like we need to update dist.jswith npm run build and there's a formatting issue

@mmlb
Copy link
Contributor Author

@mmlb mmlb commented Mar 9, 2022

Hey @joshmgross yep will fix. I remember now that I expected ci to fail for this reason, I couldn't figure out how to get tests running locally.

Signed-off-by: Manuel Mendez <mmendez534@gmail.com>
@mmlb mmlb force-pushed the do-not-export-GOROOT branch from 3730c7e to 24dcf77 Compare Mar 9, 2022
@mmlb
Copy link
Contributor Author

@mmlb mmlb commented Mar 9, 2022

@joshmgross updated!

__tests__/setup-go.test.ts Outdated Show resolved Hide resolved
__tests__/setup-go.test.ts Outdated Show resolved Hide resolved
@mmlb mmlb force-pushed the do-not-export-GOROOT branch from 24dcf77 to 3f602c0 Compare Mar 10, 2022
@mmlb
Copy link
Contributor Author

@mmlb mmlb commented Mar 10, 2022

Thanks for the review to both of you. I'm not a ts/js dev so had no clue how to run/test this and was letting CI do, but using the npm run build as a hint I figured out npm run test :D.

@joshmgross
Copy link
Member

@joshmgross joshmgross commented Mar 10, 2022

It looks like CI is failing for validating setup-go with versions < 1.9 that still need GOROOT - https://github.com/actions/setup-go/runs/5500146663?check_suite_focus=true

It seems like we should still export GOROOT if go-version is < 1.9.
@mmlb can you made those changes? If you're not comfortable enough with JS/TS, I'd be happy to help out.

__tests__/setup-go.test.ts Outdated Show resolved Hide resolved
This has not been necessary since [Go 1.9](https://go.dev/doc/go1.9#goroot) at
least (although clunky to do so then) but definitely not since
[Go 1.10](https://go.dev/doc/go1.10#goroot).

This is cargo culting code that is more than 2 years out of date and runs into
issues when multiple go versions are used in an action run.

Signed-off-by: Manuel Mendez <mmendez534@gmail.com>
@mmlb mmlb force-pushed the do-not-export-GOROOT branch from 3f602c0 to 83124a1 Compare Mar 11, 2022
@mmlb mmlb requested a review from as a code owner Mar 11, 2022
@mmlb
Copy link
Contributor Author

@mmlb mmlb commented Mar 11, 2022

It looks like CI is failing for validating setup-go with versions < 1.9 that still need GOROOT - actions/setup-go/runs/5500146663?check_suite_focus=true

It seems like we should still export GOROOT if go-version is < 1.9. @mmlb can you made those changes? If you're not comfortable enough with JS/TS, I'd be happy to help out.

I'd appreciate if you took over on that change.

Side question: go1.8 is > 5years old, whats the policy for setup-go dropping versions? Is it "don't, unless really really necessary"?

@joshmgross
Copy link
Member

@joshmgross joshmgross commented Mar 11, 2022

Side question: go1.8 is > 5years old, whats the policy for setup-go dropping versions? Is it "don't, unless really really necessary"?

I don't know if we have an explicit policy at this point - we'd need more data to understand what versions users need in their workflows.

__tests__/setup-go.test.ts Outdated Show resolved Hide resolved
@joshmgross joshmgross changed the title Do not export GOROOT Do not export GOROOT for Go versions >= 1.9 Mar 14, 2022
@joshmgross joshmgross requested a review from brcrista Mar 14, 2022
Copy link
Contributor Author

@mmlb mmlb left a comment

thanks for taking this up, just one small nit.

__tests__/setup-go.test.ts Outdated Show resolved Hide resolved
@mmlb
Copy link
Contributor Author

@mmlb mmlb commented Mar 14, 2022

lgtm, for whatever thats worth :D

@brcrista brcrista merged commit 7572680 into actions:main Mar 17, 2022
34 checks passed
@brcrista
Copy link
Contributor

@brcrista brcrista commented Mar 17, 2022

Thanks @mmlb !

@mmlb mmlb deleted the do-not-export-GOROOT branch Mar 18, 2022
tamird added a commit to petermattis/goid that referenced this issue May 12, 2022
This is no longer exported as of setup-go@v3.1.0. See
actions/setup-go#175.
tamird added a commit to petermattis/goid that referenced this issue May 12, 2022
This is no longer exported as of setup-go@v3.1.0. See
actions/setup-go#175.
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.

None yet

3 participants