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

Cleanup #11

Merged
merged 8 commits into from
Aug 9, 2018
Merged

Cleanup #11

merged 8 commits into from
Aug 9, 2018

Conversation

TWinsnes
Copy link
Contributor

@TWinsnes TWinsnes commented Aug 8, 2018

Cleaned up the repository to make it a bit more polished, added in a few bits and pieces around how to contribute, and more details about how to work with the application

ASSESSMENT.md Outdated

- Must be able to start from a cloned git repo.
- Must document any pre-requisites clearly.
- Must be contained within a GitHub project.

Choose a reason for hiding this comment

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

Change this to GitHub Repository as GitHub does have the concept of "Projects", which is not what we mean

ASSESSMENT.md Outdated

#### Security

- Network segmentation

Choose a reason for hiding this comment

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

Add (if applicable to the implementation), as someone might run this in Fargate or something where it's not really as relevant

#### Simplicity

- No superfluous dependencies
- Do not over engineer the solution

Choose a reason for hiding this comment

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

overengineer


Please note we have a code of conduct, please follow it in all your interactions with the project.

## Pull Request Process

Choose a reason for hiding this comment

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

Consider adding a pull request template

@@ -32,7 +32,7 @@ var versionCmd = &cobra.Command{
Short: "Displays the current version",
Long: `Displays the current version of the application`,
Run: func(cmd *cobra.Command, args []string) {
fmt.Println("Version: 0.2.3-pre-release")
fmt.Println("Version: 0.2.4")

Choose a reason for hiding this comment

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

In Cobra, the version command should just be an attribute of the root cobra.Command see example here

doc/readme.md Outdated

The application itself is a React based single page application (SPA) with an API backend and a postgres database used for data persistence. It's been designed to be completely stateless and will deploy into most types of environments, be it container based or VM based.

## Deploying into the cloud - scaffolds
Copy link

@nathandines nathandines Aug 9, 2018

Choose a reason for hiding this comment

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

This is more an architectural point, but I'm not sure we should provide the scaffolding; We want to see how modularly they can build a solution, and this also introduces too much complexity for them to get up and running. I reckon scrap it and let them provide a self-contained solution which we can spin up.

@@ -16,107 +16,16 @@

Choose a reason for hiding this comment

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

should add a title here # TechTestApp

Choose a reason for hiding this comment

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

Also, I think bring doc/readme.md back into here. Otherwise, put a couple of links to doc/readme.md and ASSESSMENT.md in here somewhere

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to split the readme for the app from the test, why I went with the doc/readme.md. It's already linked at line 23

There is already a title in the ./readme.md file, # Vibrato Tech Test or do you think it should be changed?

doc/readme.md Outdated

To create a new release, update `../cmd/version.go` with the new version and merge that into the master branch.

The commit message on the merge, will be the releas message, so make sure it contains the release notes.

Choose a reason for hiding this comment

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

Typo on release


Releases are deployed and managed through github, it's an automated process that is executed through the CI solution

To create a new release, update `../cmd/version.go` with the new version and merge that into the master branch.

Choose a reason for hiding this comment

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

Ideally should use the Cobra-native method mentioned above

doc/readme.md Outdated

A tag will be created on the master branch if the build and release is successful.

We use semver for versioning, `major.minor.patch[-pre-release]` and the CI solution has been configured to take note of the `-pre-release` tag of the version and upload it as a pre-release in git if it's included. So to release a new full release, make sure to not include `-pre-release` and visa versa.

Choose a reason for hiding this comment

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

vice versa

@TWinsnes TWinsnes merged commit c3672cf into master Aug 9, 2018
@TWinsnes TWinsnes deleted the cleanup branch August 9, 2018 05:18
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.

2 participants