Skip to content

chore: read environment addons from the workspace#4147

Merged
mergify[bot] merged 17 commits intoaws:mainlinefrom
Lou1415926:env/addon/workspace/read-env-addons
Nov 8, 2022
Merged

chore: read environment addons from the workspace#4147
mergify[bot] merged 17 commits intoaws:mainlinefrom
Lou1415926:env/addon/workspace/read-env-addons

Conversation

@Lou1415926
Copy link
Copy Markdown
Contributor

@Lou1415926 Lou1415926 commented Nov 1, 2022

This PR implements methods to read environment addons from the workspace in copilot/environments/addons folder.

Related: #4219

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the Apache 2.0 License.

@Lou1415926 Lou1415926 requested a review from a team as a code owner November 1, 2022 18:59
@Lou1415926 Lou1415926 requested review from efekarakus and removed request for a team November 1, 2022 18:59
@Lou1415926 Lou1415926 added the do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. label Nov 1, 2022
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Nov 1, 2022

Codecov Report

Merging #4147 (534fd24) into mainline (7a86fde) will increase coverage by 0.02%.
The diff coverage is 82.69%.

@@             Coverage Diff              @@
##           mainline    #4147      +/-   ##
============================================
+ Coverage     69.21%   69.24%   +0.02%     
============================================
  Files           250      250              
  Lines         35885    35927      +42     
  Branches        264      264              
============================================
+ Hits          24838    24876      +38     
- Misses         9854     9856       +2     
- Partials       1193     1195       +2     
Impacted Files Coverage Δ
internal/pkg/addon/addons.go 83.67% <70.00%> (-2.70%) ⬇️
internal/pkg/workspace/workspace.go 71.45% <90.62%> (+2.17%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@huanjani huanjani removed the do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. label Nov 1, 2022
Copy link
Copy Markdown
Contributor

@efekarakus efekarakus left a comment

Choose a reason for hiding this comment

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

Thanks for the great test cases

Let me know what you think of the suggestion below!

Comment thread internal/pkg/addon/addons.go Outdated
Comment on lines +34 to +35
ReadWorkloadAddonsDir(svcName string) ([]string, error)
ReadWorkloadAddon(svcName, fileName string) ([]byte, error)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Instead of ReadWorkloadAddonsDir vs ReadEnvAddonsDir, what do you think of the following API?

package workspace

// ListAddonsFiles returns a list of file paths to all the addons templates defined under the dir.
func (ws *Workspace) ListAddonsFiles(dirPath string) ([]string, error)

// ReadAddonsFile reads the addons template file under fpath.
func (ws *Workspace) ReadAddonsFile(fpath string) ([]byte, error)

// EnvAddonsPath returns the addons/ directory file path for environments.
func (ws *Workspace) EnvAddonsPath() string

// WorkloadAddonsPath returns the addons/ directory file path for a given workload.
func (ws *Workspace) WorkloadAddonsPath(name string) string

The idea is to have a generic ListAddonsFiles that can be given as input the EnvAddonsPath or WorkloadAddonsPath.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Addressed!

I actually had:

WorkloadAddonsPath(name string) (string, error)
WorkloadAddonFilePath(wkldName, fName string) (string, error)
EnvAddonsPath() string
EnvAddonFilePath() string
ListFiles(dirPath string) ([]string, error)
ReadFile(fPath string) ([]byte, error)

So that we are still letting the workspace package to determine how an addon file is arranged under an addons/ directory. lmk how you like it!

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Nov 2, 2022

🍕 Here are the new binary sizes!

Name New size (kiB) v1.23.0 size (kiB) Delta (%)
macOS (amd) 47812 47548 +0.56
macOS (arm) 47812 48200 ❤️ -0.80
linux (amd) 42044 41820 +0.54
linux (arm) 42044 41220 🥺 +2.00
windows (amd) 38876 38664 +0.55

@Lou1415926 Lou1415926 force-pushed the env/addon/workspace/read-env-addons branch from 3f74dce to 3b02b15 Compare November 7, 2022 22:43
@Lou1415926 Lou1415926 added the WIP Pull requests that are being modified now. label Nov 7, 2022
@Lou1415926 Lou1415926 force-pushed the env/addon/workspace/read-env-addons branch from bf3f1d5 to 61b59e5 Compare November 7, 2022 23:29
@Lou1415926 Lou1415926 removed the WIP Pull requests that are being modified now. label Nov 8, 2022
Copy link
Copy Markdown
Contributor

@dannyrandall dannyrandall left a comment

Choose a reason for hiding this comment

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

looks great, just a nit!

Comment thread internal/pkg/addon/addons.go
Comment thread internal/pkg/workspace/workspace.go Outdated
func (ws *Workspace) ReadFile(fPath string) ([]byte, error) {
exist, err := ws.fs.Exists(fPath)
if err != nil {
return nil, fmt.Errorf("check if manifest file %s exists: %w", fPath, err)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

should probably update this error since it could be reading a manifest file or an addon file now

@dannyrandall dannyrandall added the do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. label Nov 8, 2022
func (ws *Workspace) ReadAddonsDir(svcName string) ([]string, error) {
// EnvAddonsPath returns the addons/ directory file path for environments.
func (ws *Workspace) EnvAddonsPath() (string, error) {
copilotPath, err := ws.copilotDirPath()
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I wonder if we could have gotten-rid of so many of our errors if we had a different approach to New in our package. For example:

package workspace

// Workspace is a created copilot/ directory in a git repository.
type Workspace struct{...}

// Use returns an existing workspace and replaces the old "New" function. 
// It searches a copilot/ directory from the current wd, up to 5 levels above.
func Use() (*Workspace, error) { ... }

// Create is no longer a method and instead is a function that creates a new *Workspace in the current working directory for appName.
func Create(appName string) (*Workspace, error)

With the above API, I think we can completely ditch the copilotDirPath error because *Workspace will have the value stored in a field.

Let me know what you think of the above, if it sounds good we can do it as a follow-up to this PR?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Makes sense to me!

We can use Use() to do what've been done by copilotDirPath so that the rest of the operations can proceed with the assumption that we are in a worksapce.

@Lou1415926 Lou1415926 removed the do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. label Nov 8, 2022
@mergify mergify Bot merged commit b956464 into aws:mainline Nov 8, 2022
mergify Bot pushed a commit that referenced this pull request Nov 11, 2022
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 #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.
KollaAdithya pushed a commit to KollaAdithya/copilot-cli that referenced this pull request Nov 21, 2022
This PR implements methods to read environment addons from the workspace in `copilot/environments/addons` folder.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the Apache 2.0 License.
KollaAdithya pushed a commit to KollaAdithya/copilot-cli that referenced this pull request Nov 21, 2022
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants