-
Notifications
You must be signed in to change notification settings - Fork 335
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
Migrate knative.dev/hack/shell to knative.dev/pkg/test/shell #2856
Merged
+749
−0
Merged
Changes from 4 commits
Commits
Show all changes
12 commits
Select commit
Hold shift + click to select a range
88698c0
Move shell package from knative.dev/hack under test/shell
mgencur 974d18b
Provide Streams for shell executor that writes to t.Log
mgencur 3cfcbc6
Remove temporary comment
mgencur f6e0044
Fix style
mgencur 25f0adf
Fix lint
mgencur 64dda1e
Do not fail when writing to stderr
mgencur 551d95b
Fix arguments for Logf
mgencur 7e238a1
Fix comments
mgencur ae89365
NewExecutor function requires TestingT and ProjectLocation
mgencur b0c1be6
Move pkg/test/shell under pkg/test/upgrade/shell
mgencur 3abb743
ProjectLocation is always passed
mgencur 270ad20
Remove redundant file
mgencur File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,45 @@ | ||
/* | ||
Copyright 2020 The Knative Authors | ||
|
||
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 shell_test | ||
|
||
import ( | ||
"strings" | ||
"testing" | ||
) | ||
|
||
type assertions struct { | ||
t *testing.T | ||
} | ||
|
||
func (a assertions) NoError(err error) { | ||
if err != nil { | ||
a.t.Error(err) | ||
} | ||
} | ||
|
||
func (a assertions) Contains(haystack, needle string) { | ||
if !strings.Contains(haystack, needle) { | ||
a.t.Errorf("wanted to \ncontain: %#v\n in: %#v", | ||
needle, haystack) | ||
} | ||
} | ||
|
||
func (a assertions) Equal(want, got string) { | ||
if got != want { | ||
a.t.Errorf("want: %#v\n got:%#v", want, got) | ||
} | ||
} |
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
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,213 @@ | ||||||
/* | ||||||
Copyright 2020 The Knative Authors | ||||||
|
||||||
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 shell | ||||||
|
||||||
import ( | ||||||
"bytes" | ||||||
"errors" | ||||||
"fmt" | ||||||
"os" | ||||||
"os/exec" | ||||||
"strings" | ||||||
"testing" | ||||||
"time" | ||||||
) | ||||||
|
||||||
const ( | ||||||
defaultLabelOut = "[OUT]" | ||||||
defaultLabelErr = "[ERR]" | ||||||
executeMode = 0700 | ||||||
) | ||||||
|
||||||
// ErrNoProjectLocation is returned if user didnt provided the project location. | ||||||
var ErrNoProjectLocation = errors.New("project location isn't provided") | ||||||
|
||||||
// NewExecutor creates a new executor from given config. | ||||||
func NewExecutor(config ExecutorConfig) Executor { | ||||||
configureDefaultValues(&config) | ||||||
return &streamingExecutor{ | ||||||
ExecutorConfig: config, | ||||||
} | ||||||
} | ||||||
|
||||||
// TestingTStreams returns Streams which writes to t.Log and marks | ||||||
// the test as failed if anything is written to Streams.Err. | ||||||
func TestingTStreams(t testing.TB) Streams { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
return Streams{ | ||||||
Out: testingWriter{t: t}, | ||||||
Err: testingWriter{t: t, markFailed: true}, | ||||||
} | ||||||
} | ||||||
|
||||||
// RunScript executes a shell script with args. | ||||||
func (s *streamingExecutor) RunScript(script Script, args ...string) error { | ||||||
err := validate(s.ExecutorConfig) | ||||||
if err != nil { | ||||||
return err | ||||||
} | ||||||
cnt := script.scriptContent(s.ProjectLocation, args) | ||||||
return withTempScript(cnt, func(bin string) error { | ||||||
return stream(bin, s.ExecutorConfig, script.Label) | ||||||
}) | ||||||
} | ||||||
|
||||||
// RunFunction executes a shell function with args. | ||||||
func (s *streamingExecutor) RunFunction(fn Function, args ...string) error { | ||||||
err := validate(s.ExecutorConfig) | ||||||
if err != nil { | ||||||
return err | ||||||
} | ||||||
cnt := fn.scriptContent(s.ProjectLocation, args) | ||||||
return withTempScript(cnt, func(bin string) error { | ||||||
return stream(bin, s.ExecutorConfig, fn.Label) | ||||||
}) | ||||||
} | ||||||
|
||||||
type streamingExecutor struct { | ||||||
ExecutorConfig | ||||||
} | ||||||
|
||||||
func validate(config ExecutorConfig) error { | ||||||
if config.ProjectLocation == nil { | ||||||
return ErrNoProjectLocation | ||||||
} | ||||||
return nil | ||||||
} | ||||||
|
||||||
func configureDefaultValues(config *ExecutorConfig) { | ||||||
if config.Out == nil { | ||||||
config.Out = os.Stdout | ||||||
} | ||||||
if config.Err == nil { | ||||||
config.Err = os.Stderr | ||||||
} | ||||||
if config.LabelOut == "" { | ||||||
config.LabelOut = defaultLabelOut | ||||||
} | ||||||
if config.LabelErr == "" { | ||||||
config.LabelErr = defaultLabelErr | ||||||
} | ||||||
if config.Environ == nil { | ||||||
config.Environ = os.Environ() | ||||||
} | ||||||
if !config.SkipDate && config.DateFormat == "" { | ||||||
config.DateFormat = time.StampMilli | ||||||
} | ||||||
if config.PrefixFunc == nil { | ||||||
config.PrefixFunc = defaultPrefixFunc | ||||||
} | ||||||
} | ||||||
|
||||||
func stream(bin string, cfg ExecutorConfig, label string) error { | ||||||
c := exec.Command(bin) | ||||||
c.Env = cfg.Environ | ||||||
c.Stdout = NewPrefixer(cfg.Out, prefixFunc(StreamTypeOut, label, cfg)) | ||||||
c.Stderr = NewPrefixer(cfg.Err, prefixFunc(StreamTypeErr, label, cfg)) | ||||||
return c.Run() | ||||||
} | ||||||
|
||||||
func prefixFunc(st StreamType, label string, cfg ExecutorConfig) func() string { | ||||||
return func() string { | ||||||
return cfg.PrefixFunc(st, label, cfg) | ||||||
} | ||||||
} | ||||||
|
||||||
func defaultPrefixFunc(st StreamType, label string, cfg ExecutorConfig) string { | ||||||
sep := " " | ||||||
var buf []string | ||||||
if !cfg.SkipDate { | ||||||
dt := time.Now().Format(cfg.DateFormat) | ||||||
buf = append(buf, dt) | ||||||
} | ||||||
buf = append(buf, label) | ||||||
switch st { | ||||||
case StreamTypeOut: | ||||||
buf = append(buf, cfg.LabelOut) | ||||||
case StreamTypeErr: | ||||||
buf = append(buf, cfg.LabelErr) | ||||||
} | ||||||
return strings.Join(buf, sep) + sep | ||||||
} | ||||||
|
||||||
func withTempScript(contents string, fn func(bin string) error) error { | ||||||
tmpfile, err := os.CreateTemp("", "shellout-*.sh") | ||||||
if err != nil { | ||||||
return err | ||||||
} | ||||||
_, err = tmpfile.WriteString(contents) | ||||||
if err != nil { | ||||||
return err | ||||||
} | ||||||
err = tmpfile.Chmod(executeMode) | ||||||
if err != nil { | ||||||
return err | ||||||
} | ||||||
err = tmpfile.Close() | ||||||
if err != nil { | ||||||
return err | ||||||
} | ||||||
defer func() { | ||||||
// clean up | ||||||
_ = os.Remove(tmpfile.Name()) | ||||||
}() | ||||||
|
||||||
return fn(tmpfile.Name()) | ||||||
} | ||||||
|
||||||
func (fn *Function) scriptContent(location ProjectLocation, args []string) string { | ||||||
return fmt.Sprintf(`#!/usr/bin/env bash | ||||||
|
||||||
set -Eeuo pipefail | ||||||
|
||||||
cd "%s" | ||||||
source %s | ||||||
|
||||||
%s %s | ||||||
`, location.RootPath(), fn.ScriptPath, fn.FunctionName, quoteArgs(args)) | ||||||
} | ||||||
|
||||||
func (sc *Script) scriptContent(location ProjectLocation, args []string) string { | ||||||
return fmt.Sprintf(`#!/usr/bin/env bash | ||||||
|
||||||
set -Eeuo pipefail | ||||||
|
||||||
cd "%s" | ||||||
%s %s | ||||||
`, location.RootPath(), sc.ScriptPath, quoteArgs(args)) | ||||||
} | ||||||
|
||||||
func quoteArgs(args []string) string { | ||||||
quoted := make([]string, len(args)) | ||||||
for i, arg := range args { | ||||||
quoted[i] = "\"" + strings.ReplaceAll(arg, "\"", "\\\"") + "\"" | ||||||
} | ||||||
return strings.Join(quoted, " ") | ||||||
} | ||||||
|
||||||
func (w testingWriter) Write(p []byte) (n int, err error) { | ||||||
n = len(p) | ||||||
|
||||||
// Strip trailing newline because t.Log always adds one. | ||||||
p = bytes.TrimRight(p, "\n") | ||||||
|
||||||
w.t.Logf("%s", p) | ||||||
if w.markFailed { | ||||||
w.t.Fail() | ||||||
} | ||||||
|
||||||
return n, nil | ||||||
} |
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer to create an API that can't be misused. We probably want to force to pass the
TestingT
, we shouldn't allow people just switch without this and continue to output toos.Stdout
, andos.Stderr
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The API
shell.NewExecutor
is general. It doesn't have to be used only in upgrade tests and doesn't always have to log to testing.T. Somebody can use it to log to normal stdout and stderr. That's why I keep it optional.I will fix using this in a few repositories, at least Serving, Eventing, EventingKafkaBroker. I don't think it's a concern that somebody will migrate to the new package but will not use this new out/err. It's a narrow set of repositories. People need to know what they're doing, I think forcing this
func NewExecutor(t TestingT, config ExecutorConfig) Executor {
will just hide the fact. And it's also forcing users to pass testing.T even if they don't want to log to testing.T.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shell executor is only allowed to be used in testing, see: knative/hack#32
And if so, it doesn't make sense to log to stdout/stderr in tests, as knative/serving#14461 clearly shows.
This is why I wanted a different API, fixing the project location, at the same time. I propose to have:
Such API clearly says which params are required, and also allows customization if needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow. So this package is deprecated from the beginning? I think we're trying to do too many things with this fix. I like the previous API. From the example above it's not clear how ExecutorConfig is passed to the NewExecutor and it's increasing the num of parameters.
There would be further improvements needed:
Since this API is deprecated I don't see a point in improving it much. The goal is to fix the bug with logging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a saying in 🇵🇱 - "Makeshift is the most durable."
The original PR was done in Nov, 2020 as a temporary solution. And it still is.
However, there is a way out of this mess. The Productivity WG is currently pushing this epic: knative/hack#254. In the process, at some point, it may be feasible to rewrite these install shell functions into proper Go-native packages. Those packages can be used directly in upgrade testing, so this package could be removed permanently, as intended.
But it will take time to get there.
So I think it's worth taking some extra steps to make it less buggy. Maybe people will start new projects and run into the same problem. If we change the API to not allow that, that would prevent those potential bugs.
I like both of your suggestions: moving the package to
knative.dev/pkg/test/upgrade/shell
and removing theStreams
fromExecutorConfig
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. I pushed the changes. I left
Streams
within ExecutorConfig because it is the place where the stream configuration is held, it's later used by the executor.