Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 8 additions & 4 deletions internal/deputil/url.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,15 @@

package deputil

import "net/http"
import (
"net/http"

// URLChecker returns url if it's status code is 200, otherwise returns empty string
func URLChecker(url string) string {
resp, err := http.Get(url)
"github.com/slackapi/slack-cli/internal/slackhttp"
)

// URLChecker returns url if its status code is 200, otherwise returns empty string
Copy link
Member

Choose a reason for hiding this comment

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

πŸ” I'm not sure if this was included in the followup plans, but we might also want to return an error here for continued improvements to error messages?

It's not clear to me if .Get returns separate errors for the specific error cases:

  • Internet broke
  • URL is missing

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 had the same thought but let it slide. I agree, let's follow-up with returning an error from URLChecker πŸ‘πŸ»

func URLChecker(httpClient slackhttp.HTTPClient, url string) string {
resp, err := httpClient.Get(url)
Copy link
Member Author

Choose a reason for hiding this comment

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

note: A future PR should update this to .Head. This will require adding Head to our HTTPClient interface as well.

Copy link
Member

Choose a reason for hiding this comment

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

@mwbrooks Great thinking! For templates that vendor dependencies I imagine this being an expensive operation and I fear rate limits might've been blocking downloads in CI sometimes as well πŸ˜“

It's so good to know of this follow up! 🧠 ✨

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 fear rate limits might've been blocking downloads in CI sometimes as well πŸ˜“

🧠 Hopefully this reduces some flakiness!

if err != nil {
return ""
}
Expand Down
49 changes: 45 additions & 4 deletions internal/deputil/url_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,15 +15,56 @@
package deputil

import (
"fmt"
"net/http"
"testing"

"github.com/slackapi/slack-cli/internal/slackhttp"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/mock"
)

func Test_URLChecker(t *testing.T) {
url := URLChecker("https://github.com/slack-samples/deno-starter-template")
assert.Equal(t, "https://github.com/slack-samples/deno-starter-template", url, "should return url when url is valid")
tests := map[string]struct {
Copy link
Member Author

Choose a reason for hiding this comment

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

note: Reimplemented the tests as a table test and extended the cover. This function now has 100% test coverage. πŸ’―

url string
expectedURL string
setupHTTPClientMock func(*slackhttp.HTTPClientMock)
}{
"Returns the URL when the HTTP status code is http.StatusOK": {
url: "https://github.com/slack-samples/deno-starter-template",
expectedURL: "https://github.com/slack-samples/deno-starter-template",
setupHTTPClientMock: func(httpClientMock *slackhttp.HTTPClientMock) {
resOK := slackhttp.MockHTTPResponse(http.StatusOK, "OK")
httpClientMock.On("Get", mock.Anything).Return(resOK, nil)
},
},
"Returns an empty string when the HTTP status code is not 200": {
url: "https://github.com/slack-samples/template-not-found",
expectedURL: "",
setupHTTPClientMock: func(httpClientMock *slackhttp.HTTPClientMock) {
resNotFound := slackhttp.MockHTTPResponse(http.StatusNotFound, "Not Found")
httpClientMock.On("Get", mock.Anything).Return(resNotFound, nil)
},
},
"Returns an empty string when the HTTPClient has an error": {
url: "invalid_url",
expectedURL: "",
setupHTTPClientMock: func(httpClientMock *slackhttp.HTTPClientMock) {
httpClientMock.On("Get", mock.Anything).Return(nil, fmt.Errorf("HTTPClient error"))
},
},
}
for name, tt := range tests {
t.Run(name, func(t *testing.T) {
// Create mocks
httpClientMock := &slackhttp.HTTPClientMock{}
tt.setupHTTPClientMock(httpClientMock)

url = URLChecker("fake_url")
assert.Equal(t, "", url, "should return empty string when url is invalid")
// Execute
url := URLChecker(httpClientMock, tt.url)

// Assertions
assert.Equal(t, tt.expectedURL, url)
})
}
}
15 changes: 8 additions & 7 deletions internal/pkg/create/create.go
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ import (
"github.com/slackapi/slack-cli/internal/logger"
"github.com/slackapi/slack-cli/internal/shared"
"github.com/slackapi/slack-cli/internal/slackerror"
"github.com/slackapi/slack-cli/internal/slackhttp"
"github.com/slackapi/slack-cli/internal/slacktrace"
"github.com/slackapi/slack-cli/internal/style"
"github.com/spf13/afero"
Expand Down Expand Up @@ -225,7 +226,8 @@ func createApp(ctx context.Context, dirPath string, template Template, gitBranch
if template.isGit {
doctorSection, err := doctor.CheckGit(ctx)
if doctorSection.HasError() || err != nil {
zipFileURL := generateGitZipFileURL(template.path, gitBranch)
httpClient := slackhttp.NewHTTPClient(slackhttp.HTTPClientOptions{})
zipFileURL := generateGitZipFileURL(httpClient, template.path, gitBranch)
if zipFileURL == "" {
return slackerror.New(slackerror.ErrGitZipDownload)
}
Expand Down Expand Up @@ -520,18 +522,17 @@ func InstallProjectDependencies(
return outputs
}

// generateGitZipFileURL will return template's GitHub zip file download link
// In the future, this function can be extended to support other Git hosts, such as GitLab.
// TODO, @cchensh, we should get prepared for other non-Git hosts and refactor the create pkg
func generateGitZipFileURL(templateURL string, gitBranch string) string {
// generateGitZipFileURL will return the GitHub zip URL for a templateURL.
func generateGitZipFileURL(httpClient slackhttp.HTTPClient, templateURL string, gitBranch string) string {
zipURL := strings.ReplaceAll(templateURL, ".git", "") + "/archive/refs/heads/"

if gitBranch == "" {
mainURL := zipURL + "main.zip"
masterURL := zipURL + "master.zip"
zipURL = deputil.URLChecker(mainURL)

zipURL = deputil.URLChecker(httpClient, mainURL)
if zipURL == "" {
zipURL = deputil.URLChecker(masterURL)
zipURL = deputil.URLChecker(httpClient, masterURL)
}
} else {
zipURL = zipURL + gitBranch + ".zip"
Copy link
Member Author

Choose a reason for hiding this comment

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

note: Kinda funny that we never apply URLChecker to custom branches. Work for the future!

Copy link
Member

Choose a reason for hiding this comment

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

@mwbrooks I was wondering of possible reason for this, but the points around how we can improve error messages for missing URLs outweigh the missing checks here I hope πŸͺ„

Expand Down
75 changes: 65 additions & 10 deletions internal/pkg/create/create_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,15 @@ package create

import (
"fmt"
"net/http"
"path/filepath"
"testing"

"github.com/slackapi/slack-cli/internal/config"
"github.com/slackapi/slack-cli/internal/experiment"
"github.com/slackapi/slack-cli/internal/shared"
"github.com/slackapi/slack-cli/internal/slackcontext"
"github.com/slackapi/slack-cli/internal/slackhttp"
"github.com/spf13/afero"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/mock"
Expand Down Expand Up @@ -75,18 +77,71 @@ func TestGetAvailableDirectory(t *testing.T) {
}

func Test_generateGitZipFileURL(t *testing.T) {
url := generateGitZipFileURL("https://github.com/slack-samples/deno-starter-template", "pre-release-0316")
assert.Equal(t, "https://github.com/slack-samples/deno-starter-template/archive/refs/heads/pre-release-0316.zip", url, "should return zip download link with branch")

url = generateGitZipFileURL("https://github.com/slack-samples/deno-starter-template", "")
assert.Equal(t, "https://github.com/slack-samples/deno-starter-template/archive/refs/heads/main.zip", url, "should return zip download link with main")
tests := map[string]struct {
Copy link
Member Author

Choose a reason for hiding this comment

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

note: Reimplemented the tests as a table test and extended the cover. This function now has 100% test coverage. πŸ’―

Copy link
Member

Choose a reason for hiding this comment

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

@mwbrooks Huge unlock for test stabilities all around here πŸ” ✨

While it'd be so surprising to me, I can imagine missing branches might cause some of the past tests to fail πŸ˜‰

templateURL string
gitBranch string
expectedURL string
setupHTTPClientMock func(*slackhttp.HTTPClientMock)
}{
"Returns the zip URL using the main branch when no branch is provided": {
templateURL: "https://github.com/slack-samples/deno-starter-template",
gitBranch: "",
expectedURL: "https://github.com/slack-samples/deno-starter-template/archive/refs/heads/main.zip",
setupHTTPClientMock: func(httpClientMock *slackhttp.HTTPClientMock) {
res := slackhttp.MockHTTPResponse(http.StatusOK, "OK")
httpClientMock.On("Get", mock.Anything).Return(res, nil)
},
},
"Returns the zip URL using the master branch when no branch is provided and main branch doesn't exist": {
templateURL: "https://github.com/slack-samples/deno-starter-template",
gitBranch: "",
expectedURL: "https://github.com/slack-samples/deno-starter-template/archive/refs/heads/master.zip",
setupHTTPClientMock: func(httpClientMock *slackhttp.HTTPClientMock) {
res := slackhttp.MockHTTPResponse(http.StatusOK, "OK")
httpClientMock.On("Get", "https://github.com/slack-samples/deno-starter-template/archive/refs/heads/main.zip").Return(nil, fmt.Errorf("HttpClient error"))
httpClientMock.On("Get", "https://github.com/slack-samples/deno-starter-template/archive/refs/heads/master.zip").Return(res, nil)
},
},
"Returns the zip URL using the specified branch when a branch is provided": {
templateURL: "https://github.com/slack-samples/deno-starter-template",
gitBranch: "pre-release-0316",
expectedURL: "https://github.com/slack-samples/deno-starter-template/archive/refs/heads/pre-release-0316.zip",
setupHTTPClientMock: func(httpClientMock *slackhttp.HTTPClientMock) {
res := slackhttp.MockHTTPResponse(http.StatusOK, "OK")
httpClientMock.On("Get", mock.Anything).Return(res, nil)
},
},
"Returns an empty string when the HTTP status code is not 200": {
templateURL: "https://github.com/slack-samples/deno-starter-template",
gitBranch: "",
expectedURL: "",
setupHTTPClientMock: func(httpClientMock *slackhttp.HTTPClientMock) {
res := slackhttp.MockHTTPResponse(http.StatusNotFound, "Not Found")
httpClientMock.On("Get", mock.Anything).Return(res, nil)
},
},
"Returns an empty string when the HTTPClient has an error": {
templateURL: "https://github.com/slack-samples/deno-starter-template",
gitBranch: "",
expectedURL: "",
setupHTTPClientMock: func(httpClientMock *slackhttp.HTTPClientMock) {
httpClientMock.On("Get", mock.Anything).Return(nil, fmt.Errorf("HTTPClient error"))
},
},
}
for name, tt := range tests {
t.Run(name, func(t *testing.T) {
// Create mocks
httpClientMock := &slackhttp.HTTPClientMock{}
tt.setupHTTPClientMock(httpClientMock)

// TODO - We should mock the `deputil.URLChecker` HTTP request so that the unit test is not dependent on the network activity and repo configuration
url = generateGitZipFileURL("https://github.com/google/uuid", "")
assert.Equal(t, "https://github.com/google/uuid/archive/refs/heads/master.zip", url, "should return zip download link with 'master' when 'main' branch doesn't exist")
// Execute
url := generateGitZipFileURL(httpClientMock, tt.templateURL, tt.gitBranch)

url = generateGitZipFileURL("fake_url", "")
assert.Equal(t, "", url, "should return empty string when url is invalid")
// Assertions
assert.Equal(t, tt.expectedURL, url)
})
}
}

func TestCreateGitArgs(t *testing.T) {
Expand Down
71 changes: 71 additions & 0 deletions internal/slackhttp/http.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
// Copyright 2022-2025 Salesforce, Inc.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package slackhttp

import (
"crypto/tls"
"net/http"
"time"
)

const (
// defaultTotalTimeout is the default timeout for the life of the request.
defaultTotalTimeout = 30 * time.Second
)

// HTTPClient interface for http.Client.
type HTTPClient interface {
Do(req *http.Request) (*http.Response, error)
Get(url string) (*http.Response, error)
}

// HTTPClientOptions allows for the customization of a http.Client.
type HTTPClientOptions struct {
AttemptTimeout *time.Duration // AttemptTimeout is how long each request waits before cancelled.
Backoff *time.Duration // Backoff is a constant duration to wait between requests.
Retries int // Retries for a request, if 0 then no retry logic is added.
RetryErrorCodes []string // RetryErrorCode is list of error codes to retry on.
RetryStatusCodes []int // RetryStatusCode is list of http status codes to retry on.
SkipTLSVerify bool // SkipTLSVerify will skip verifying the host's certificate.
TotalTimeOut time.Duration // TotalTimeOut for the life of the request (default: defaultTotalTimeout).
}

// NewHTTPClient returns an http.Client configured with HTTPClientOptions.
func NewHTTPClient(opts HTTPClientOptions) *http.Client {
client := &http.Client{}

if opts.TotalTimeOut == 0 {
client.Timeout = defaultTotalTimeout
} else {
client.Timeout = opts.TotalTimeOut
}

var transport = http.Transport{
TLSClientConfig: &tls.Config{
InsecureSkipVerify: opts.SkipTLSVerify,
},
Proxy: http.ProxyFromEnvironment,
}

// If retries aren't specified return a non-retrying transport.
if opts.Retries == 0 {
client.Transport = &transport
return client
}

// TODO: Implement retry logic
client.Transport = &transport
return client
}
65 changes: 65 additions & 0 deletions internal/slackhttp/http_mock.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
// Copyright 2022-2025 Salesforce, Inc.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package slackhttp

import (
"io"
"net/http"
"net/http/httptest"

"github.com/stretchr/testify/mock"
)

// HTTPClientMock implements a mock for the HTTPClient interface.
type HTTPClientMock struct {
mock.Mock
}

// Do is a mock that tracks the calls to Do and returns the mocked http.Response and error.
func (m *HTTPClientMock) Do(req *http.Request) (*http.Response, error) {
args := m.Called(req)

// http.Response can be nil when an error is provided.
var httpResp *http.Response
if _httpResp, ok := args.Get(0).(*http.Response); ok {
httpResp = _httpResp
}

return httpResp, args.Error(1)
}

// Get is a mock that tracks the calls to Get and returns the mocked http.Response and error.
func (m *HTTPClientMock) Get(url string) (*http.Response, error) {
args := m.Called(url)

// http.Response can be nil when an error is provided.
var httpResp *http.Response
if _httpResp, ok := args.Get(0).(*http.Response); ok {
httpResp = _httpResp
}

return httpResp, args.Error(1)
}

// MockHTTPResponse is a helper that returns a mocked http.Response with the provided httpStatus and body.
func MockHTTPResponse(httpStatus int, body string) *http.Response {
resWriter := httptest.NewRecorder()

resWriter.WriteHeader(httpStatus)
_, _ = io.WriteString(resWriter, body)
res := resWriter.Result()

return res
}
Loading
Loading