chore: implement new api for the workspace package#4166
chore: implement new api for the workspace package#4166mergify[bot] merged 19 commits intoaws:mainlinefrom
workspace package#4166Conversation
…ion that the workspace path is already read
…ut if not present
Lou1415926
requested review from
dannyrandall
and removed request for
a team
Lou1415926
changed the title
workspace interfaceworkspace package
|
🍕 Here are the new binary sizes!
|
| workingDirAbs, err := os.Getwd() | ||
| // Use returns an existing workspace, searching for a copilot/ directory from the current wd, | ||
| // up to 5 levels above. It returns ErrWorkspaceNotFound if no copilot/ directory is found. | ||
| func Use(fs *afero.Afero, workingDirAbs string) (*Workspace, error) { |
There was a problem hiding this comment.
I didn't read the PR 😂 I thought I could leave this early feedback to see what you think:
-
Is it possible to use the
afero.Fsinterface as the input instead of the concrete type? or are we depending on some more specific functionality -
Can we make the "workingDirAbs" optional? could the constructor by default use
os.Getwdunless the user specifiedworkspace.Use(&afero.Afero{Fs: afero.NewOsFs()}, workspace.WithWorkingDir("some/other/path"))
Would that simplify the life of theclipkg?
There was a problem hiding this comment.
Addressed both!
The whole reason why we needed to pass workingDir as a param was for the unit tests - specifically, the ones that test that Copilot is able to find a workspace in a parent directory. I fixed the unit tests such that it doesn't need to do so. This way, we don't need it as a param anymore, not even as an optional param.
Codecov Report
@@ Coverage Diff @@
## mainline #4166 +/- ##
============================================
+ Coverage 69.24% 69.28% +0.04%
============================================
Files 250 250
Lines 35927 35882 -45
Branches 264 264
============================================
- Hits 24876 24862 -14
+ Misses 9856 9828 -28
+ Partials 1195 1192 -3
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
| sel: selector.NewLocalEnvironmentSelector(prompt.New(), cfgStore, ws), | ||
| caller: identity.New(defaultSess), | ||
| fs: &afero.Afero{Fs: afero.NewOsFs()}, | ||
| fs: fs, |
There was a problem hiding this comment.
This are a couple of refactors like this that are unrelated to the main purpose of this PR.
The fs field of packageEnvOpts needs to just implement afero.Fs, and afero.NewFs() already does this. We don't need the other methods in *afero.Afero.
| workspacePath, err := ws.Path() | ||
| if err != nil { | ||
| return nil, fmt.Errorf("get workspace path: %w", err) |
| require.NoError(t, err) | ||
| fs := afero.NewMemMapFs() | ||
| _ = fs.MkdirAll(fmt.Sprintf("%s/copilot", wd), 0755) | ||
| _ = afero.WriteFile(fs, fmt.Sprintf("%s/copilot/.workspace", wd), []byte(fmt.Sprintf("---\napplication: %s", "DavidsApp")), 0644) |
There was a problem hiding this comment.
Awesome improvement! 💅 but terrible app name
iamhopaul123
added
the
do-not-merge
Lou1415926
removed
the
do-not-merge
This PR implements a new API for the `workspace` package. With the new API, a workspace client that gets returned to the caller always have the `copilotDirAbs` field in it. This means that the method calls can assume non-empty value of that field. This addresses the proposal made in aws#4147 (comment) By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the Apache 2.0 License.
5 participants
This PR implements a new API for the
workspacepackage. With the new API, a workspace client that gets returned to the caller always have thecopilotDirAbsfield in it. This means that the method calls can assume non-empty value of that field.This addresses the proposal made in #4147 (comment)
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the Apache 2.0 License.