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.

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

Add workspace pkg for managing local configuration #44

Merged
merged 2 commits into from
Sep 19, 2019
Merged

Add workspace pkg for managing local configuration #44

merged 2 commits into from
Sep 19, 2019

Conversation

kohidave
Copy link
Contributor

Adding a new package for easy reading and writing of local
workspace level configuration.

This includes:

  1. Reading and writing manifest files
  2. Reading and writing a project summary file

The workspace summary is used to store the project name
(and in the future perhaps the pipeline in charge of this
workspace).

@kohidave kohidave added this to the 1. Bootstrapping milestone Sep 17, 2019
@kohidave kohidave requested review from efekarakus, hencrice, sonofachamp and a team September 17, 2019 23:29
@kohidave kohidave self-assigned this Sep 17, 2019
Copy link
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.

This was super easy to follow, I really liked the comments you added. It feels like effort was put into it :)

small nits

pkg/workspace/service.go Outdated Show resolved Hide resolved
pkg/workspace/service.go Outdated Show resolved Hide resolved
pkg/workspace/service.go Outdated Show resolved Hide resolved
pkg/workspace/service.go Outdated Show resolved Hide resolved
}

func (ws *Service) manifestDirectoryPath() (string, error) {
if ws.manifestDir != "" {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: can we add a comment here saying that we're caching the manifest dir path to avoid repeating system calls?

Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if it's better to call this function in the constructor and then have all the other functions just refer to ws.manifestDir? this way we won't have to do the additional error check in the other functions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, that's not a bad idea. sure!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, that might be a little tough because of the create case, when we expect there to not be a root directory yet =/

pkg/workspace/service.go Outdated Show resolved Hide resolved
pkg/workspace/service_test.go Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
pkg/workspace/service.go Outdated Show resolved Hide resolved
pkg/workspace/service.go Outdated Show resolved Hide resolved
pkg/workspace/service.go Outdated Show resolved Hide resolved
pkg/workspace/service.go Outdated Show resolved Hide resolved
pkg/workspace/service.go Outdated Show resolved Hide resolved
pkg/workspace/service.go Outdated Show resolved Hide resolved
Copy link
Contributor

@hencrice hencrice left a comment

Choose a reason for hiding this comment

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

Are we going to use pkg/error to wrap all our errors so that when things break we have the full stacktrace instead of just a single error?

pkg/archer/workspace.go Outdated Show resolved Hide resolved
pkg/workspace/service_test.go Outdated Show resolved Hide resolved
pkg/workspace/service_test.go Outdated Show resolved Hide resolved
pkg/workspace/service_test.go Outdated Show resolved Hide resolved

// If there isn't an existing workspace summary, create it.
var noProjectFound *ErrNoProjectAssociated
if errors.As(existingWorkspaceSummaryErr, &noProjectFound) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The future is here

}

// ManifestManager can read, write and list local manifest files.
type ManifestManager interface {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this interface be simplified to:

type Manifest interface {
    Write([]byte, string) error
    Read(string) ([]byte, error)
    List() ([]string, error)
}

Manager seems implicit via the API.

@@ -0,0 +1,43 @@
package workspace
Copy link
Contributor

Choose a reason for hiding this comment

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

missing Copyright lines

@@ -0,0 +1,379 @@
package workspace
Copy link
Contributor

Choose a reason for hiding this comment

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

Copyright lines

Adding a new package for easy reading and writing of local
workspace level configuration.

This includes:

1. Reading and Writing manifest files
2. Reading and Writing a project breadcrumb file

The project breadcrumb is used to store the project name
(and in the future perhaps the pipeline in charge of this
workspace).
@kohidave kohidave merged commit f5aab37 into aws:master Sep 19, 2019
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.

4 participants