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.

Sign up for GitHub

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

Jump to bottom

Use go template for powershell, golang and node #1095

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Use go template for powershell, golang and node #1095

wants to merge 1 commit into from

Conversation

nitishkumar71
Copy link
Contributor

@nitishkumar71 nitishkumar71 commented Jul 1, 2024 •
edited
Loading

Description

We are extending go template usage for system install apps like powershell, go and node

Motivation and Context

How Has This Been Tested?

If updating or adding a new CLI to arkade get, run:

go build && ./hack/test-tool.sh TOOL_NAME

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Documentation

  • I have updated the list of tools in README.md if (required) with ./arkade get --format markdown
  • I have updated the list of apps in README.md if (required) with ./arkade install --help

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I've read the CONTRIBUTION guide
  • I have signed-off my commits with git commit -s
  • I have tested this on arm, or have added code to prevent deployment

@nitishkumar71 nitishkumar71 force-pushed the system_tool_template branch 2 times, most recently from 5c9e788 to ab15153 Compare July 3, 2024 15:45
@nitishkumar71 nitishkumar71 marked this pull request as draft July 3, 2024 15:58
nitishkumar71
nitishkumar71 commented Jul 3, 2024
View reviewed changes
pkg/get/url_checker_test.go Outdated
@@ -14,6 +14,7 @@ func Test_CheckTools(t *testing.T) {
tools := MakeTools()
toolsToSkip := []string{
"kumactl", // S3 bucket disallow HEAD requests
"flyctl",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

need to remove this.

Copy link
Owner

Choose a reason for hiding this comment

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

OK let me know when you have..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed

nitishkumar71
nitishkumar71 commented Jul 3, 2024
View reviewed changes
pkg/get/tools.go Outdated
Repo: "go",
Name: "go",
SystemOnly: true,
Version: "1.22.4",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since go does not uses github or k8s version strategy, will it be correct to hard-code it?

Copy link
Owner

Choose a reason for hiding this comment

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

One option is to hard-code it, the other thing we could have a strategy for go and move the code from the system installer there. The version can already be determined.

@rgee0 what are your thoughts? Go is installed via system install with custom code, we are looking at using the template system instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking to execute go version detection code to be only executed when cli is actually installing the go. I am not sure, how to setup that in current code base.

Copy link
Contributor

@rgee0 rgee0 Jul 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I'd suggest a similar approach to the existing version strategies but using: https://go.dev/VERSION?m=text

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd suggest a similar approach to the exiting version strategies but using: https://go.dev/VERSION?m=text

This is implemented in system install but we are stuck around how to implement same in template approach

Copy link
Contributor

Choose a reason for hiding this comment

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

Apologies I missed The version can already be determined, maybe im also missing some context, but you're using GetDownloadURL which will take you into GetUrl which is where the existing version strategies are implemented. tool is available all the way through so a goVersionStrategy could be implemented in there in much the same way as k8s is.

@nitishkumar71 nitishkumar71 marked this pull request as ready for review July 10, 2024 15:10
@nitishkumar71 nitishkumar71 force-pushed the system_tool_template branch 2 times, most recently from c03dce7 to 6998f2b Compare July 17, 2024 08:29
@nitishkumar71 nitishkumar71 force-pushed the system_tool_template branch 2 times, most recently from 36b76f9 to 82a9a6f Compare July 17, 2024 09:28
@rgee0
Copy link
Contributor

rgee0 commented Jul 17, 2024

Thats the approach I had in mind. You can see how with any further version strategies we might benefit from looking at optimising the approach to them, but I think we're OK for the moment.

The only thought I had was whether there'd be greater safety in parsing the content returned by go.dev/VERSION through a regex to extract the version. This would defend a situation where the content might change to add additional content that precedes the existing

@nitishkumar71 nitishkumar71 force-pushed the system_tool_template branch 2 times, most recently from 5bf976f to 165b315 Compare July 20, 2024 15:51
@nitishkumar71
Copy link
Contributor Author

Thats the approach I had in mind. You can see how with any further version strategies we might benefit from looking at optimising the approach to them, but I think we're OK for the moment.

The only thought I had was whether there'd be greater safety in parsing the content returned by go.dev/VERSION through a regex to extract the version. This would defend a situation where the content might change to add additional content that precedes the existing

Changes are done.

@nitishkumar71 nitishkumar71 force-pushed the system_tool_template branch 2 times, most recently from 6484861 to 33bd535 Compare September 15, 2024 07:16
@nitishkumar71 nitishkumar71 changed the title Use go template for powershell and golang Use go template for powershell, golang and node Sep 15, 2024
@nitishkumar71 nitishkumar71 force-pushed the system_tool_template branch 3 times, most recently from bdfb12a to ae9f9cc Compare September 15, 2024 07:23
@nitishkumar71
include system url as template
299eb4f 
Signed-off-by: Nitishkumar Singh <[email protected]>

move powershell and go to tools

Signed-off-by: Nitishkumar Singh <[email protected]>

empty commit

Signed-off-by: Nitishkumar Singh <[email protected]>

implement custom version resolver

Signed-off-by: Nitishkumar Singh <[email protected]>

switch node to tools

Signed-off-by: Nitishkumar Singh <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rgee0 rgee0 rgee0 approved these changes

@alexellis alexellis Awaiting requested review from alexellis

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Use Go template for system install apps
3 participants
@nitishkumar71 @rgee0 @alexellis