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

Add Microsoft Go support #19000

Merged
merged 4 commits into from
Sep 22, 2023
Merged

Conversation

v-mohithgc
Copy link
Contributor

@v-mohithgc v-mohithgc commented Sep 20, 2023

Task name: GoToolV0

Description: Clone of #18681: Add a goSource parameter to allow use of the Microsoft Go tool and default Go tools

image

Checklist:

  • Task version was bumped - please check instruction how to do it
  • Checked that applied changes work as expected

@v-mohithgc v-mohithgc self-assigned this Sep 20, 2023
@v-mohithgc v-mohithgc added the Area:RM RM task team label Sep 20, 2023
Copy link
Contributor

@LeftTwixWand LeftTwixWand left a comment

Choose a reason for hiding this comment

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

Looks good👍

@shueybubbles
Copy link

thx a ton for working through this!
This should help a bunch of teams

Copy link

@shueybubbles shueybubbles left a comment

Choose a reason for hiding this comment

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

:shipit:

@v-mohithgc v-mohithgc merged commit 3d8da00 into microsoft:master Sep 22, 2023
Copy link
Member

@dagood dagood left a comment

Choose a reason for hiding this comment

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

Sorry I'm late to the party--I have concerns about making this a simple dropdown. For context, in #16045 I mentioned:

We don't want to change the default base URL: Azure Pipelines users outside Microsoft should continue to get the official Go binaries. (Our build of Go is only intended for use within Microsoft to meet supply chain and FIPS requirements. For more info, the Microsoft build of Go is maintained at https://github.com/microsoft/go.)

Based on the screenshot and PR it doesn't look like there's any way for a user who sees this option to get more context about what the MS Go Tools option does. Without context, I worry that this will show up to anyone using AzDO (whether internal or not) and people will choose this just because it says "MS", not knowing what it's for and not needing it. This might result in spurious issues being filed (on this repo and ours) because e.g. this option doesn't support the same set of platforms as official Go.

If any of that is wrong or not a concern for a reason I haven't thought about, please let me know. 🙂

I prefer a raw URL input because then the user needs to already know what our toolset is for and very intentionally use it. This also avoids preferential treatment towards our fork of Go--with a URL, anyone could set up their own fork of Go and use that with this task.

The microsoft/go project might change to actually wanting to "advertise" our fork with dropdowns like this in the future, but so far that doesn't fit our goals.

@@ -33,7 +33,7 @@
"name": "version",
"type": "string",
"label": "ms-resource:loc.input.label.version",
"defaultValue": "1.10",
"defaultValue": "1.19",
Copy link
Member

Choose a reason for hiding this comment

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

Go 1.21 was released on Aug 8 (after PR #18681), which means 1.19 is now EOL. 1.20 is now the oldest supported major version, so:

Suggested change
"defaultValue": "1.19",
"defaultValue": "1.20",

}
// Is there a safe way to allow fully custom URLs? How would we create the cache key?
if (goSourceInput != defaultSource && goSourceInput != microsoftSource) {
throw new Error('Only google and Microsoft sources are allowed')
Copy link
Member

Choose a reason for hiding this comment

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

Consistent capitalization?

Suggested change
throw new Error('Only google and Microsoft sources are allowed')
throw new Error('Only Google and Microsoft sources are allowed')

dagood added a commit to dagood/azure-pipelines-tasks that referenced this pull request Sep 22, 2023
@dagood
Copy link
Member

dagood commented Sep 22, 2023

I submitted a revert PR here: #19012. Sorry I didn't get a chance to review this before it was merged, or when the dropdown was originally discussed.

v-mohithgc added a commit that referenced this pull request Sep 29, 2023
* Revert "Added Microsoft Go tool support (#19000)"

This reverts commit 3d8da00.

* Update GoToolV0 to 0.229.1

* Update GoToolV0 to 0.229.1 in task.loc.json

---------

Co-authored-by: Mohith <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area:RM RM task team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants