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

Initial implementation #1

Merged
merged 51 commits into from
May 3, 2022
Merged

Initial implementation #1

merged 51 commits into from
May 3, 2022

Conversation

stigok
Copy link
Collaborator

@stigok stigok commented Apr 26, 2022

No description provided.

@stigok stigok marked this pull request as draft April 27, 2022 12:15
@stigok stigok requested a review from a team April 27, 2022 14:18
Dockerfile Outdated Show resolved Hide resolved
Dockerfile Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Show resolved Hide resolved
@stigok stigok marked this pull request as ready for review April 28, 2022 09:30
@stigok stigok requested a review from umglurf April 28, 2022 10:23
@umglurf umglurf requested a review from martensson April 28, 2022 10:24
Copy link
Contributor

@umglurf umglurf left a comment

Choose a reason for hiding this comment

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

Looks good to me now, but a gopher should also check it

@stigok stigok requested a review from mariusu April 28, 2022 11:00
Copy link

@mariusu mariusu left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link

@martensson martensson left a comment

Choose a reason for hiding this comment

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

Ser bra ut, la bare til ett par kommentarer på ting som ikke er deal breaking :)

- `MemoryStore`: in-memory store that can be populated manually
- Must be populated manually
- `GitHubStore`: queries the GitHub API for modules, version tags and SSH download URLs
- A query for module `namespace/name/provider` will return repository `namespace/name`

Choose a reason for hiding this comment

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

Om jeg ikke husker feil så snakket vi om å implementere en "generic" path som default?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

#5

- `provider` part of module URLs is not implemented. Can be set to anything at all.
- Works for a single specified org/user
- No verification for the repo actually being a Terraform repo

Choose a reason for hiding this comment

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

Hvilke typ av repos vil GithubStore regne som TF moduler? (topicFilter)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

#6


router *chi.Mux
authTokens []string
moduleStore core.ModuleStore

Choose a reason for hiding this comment

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

Ikke viktig akkurat nå, men en framtide feature kan være å støtte flere ModuleStores samtidig. 😇

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

#3

}
}

func (app *App) Health() http.HandlerFunc {

Choose a reason for hiding this comment

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

Framtide feature er å sjekke status på backends også (tilgang mot Github for eksempel)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

#4

@stigok stigok merged commit 00f4b01 into main May 3, 2022
@stigok stigok deleted the wip branch May 3, 2022 13:35
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