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

Show atlantis version in index page #57

Merged
merged 4 commits into from
Mar 14, 2018

Conversation

psalaberria002
Copy link
Contributor

@psalaberria002 psalaberria002 commented Mar 10, 2018

Works towards #33

captura de pantalla 2018-03-10 a las 19 06 57

Copy link
Member

@lkysow lkysow 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 taking this on! I've just got a couple comments around naming and placement.

server/server.go Outdated
@@ -71,6 +72,7 @@ type Config struct {
SlackToken string `mapstructure:"slack-token"`
SSLCertFile string `mapstructure:"ssl-cert-file"`
SSLKeyFile string `mapstructure:"ssl-key-file"`
Version string `mapstructure:"version"`
Copy link
Member

Choose a reason for hiding this comment

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

It's kinda annoying right now but this struct is supposed to map to the config flags and the config file. That's what the mapstructure tags are for, so it can deserialize a yaml config file.

So given that, I don't think it makes sense to include Version here, because it's not something that a user is allowed to configure. What I think we should do:

  1. Rename this struct from Config to UserConfig
  2. Rename the FlagNames struct to Config and add the Version field to that.
  3. Instead of calling it Version, let's use AtlantisVersion so it's clear it's not the terraform version or something else

server/server.go Outdated
s.IndexTemplate.Execute(w, results) // nolint: errcheck
s.IndexTemplate.Execute(w, IndexData {
Locks: lockResults,
Version: s.Version,
Copy link
Member

Choose a reason for hiding this comment

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

Version => AtlantisVersion

@@ -308,17 +311,20 @@ func (s *Server) Index(w http.ResponseWriter, _ *http.Request) {
return
}

var results []LockIndexData
var lockResults []LockIndexData
Copy link
Member

Choose a reason for hiding this comment

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

I like this rename, thanks!

@@ -24,6 +24,11 @@ type LockIndexData struct {
Time time.Time
}

type IndexData struct {
Copy link
Member

Choose a reason for hiding this comment

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

Needs a comment, ex. // IndexData holds the data for rendering the index page

@@ -24,6 +24,11 @@ type LockIndexData struct {
Time time.Time
}

type IndexData struct {
Locks []LockIndexData
Version string
Copy link
Member

Choose a reason for hiding this comment

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

AtlantisVersion

@@ -51,7 +56,7 @@ var indexTemplate = template.Must(template.New("index.html.tmpl").Parse(`
<div class="container">
<section class="header">
<a title="atlantis" href="/"><img src="/static/images/atlantis-icon.png"/></a>
<p class="title-heading">atlantis</p>
<p class="title-heading">atlantis {{ .Version }}</p>
Copy link
Member

Choose a reason for hiding this comment

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

I think this makes more sense to be in the bottom left of footer with the prefix v. I don't like having it in the title here because I think it gives the version too much prominence. If you don't want to mess with the CSS for a fixed footer that's totally fine, just LMK and I can merge your PR and then add that myself.

- Config to UserConfig
- FlagNames to Config
- Use AtlantisVersion to be clear
@psalaberria002
Copy link
Contributor Author

If you could add the version in the footer that would be awesome @lkysow

Copy link
Member

@lkysow lkysow left a comment

Choose a reason for hiding this comment

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

Will put version in the footer in a separate PR.

@lkysow lkysow merged commit d8569d3 into runatlantis:master Mar 14, 2018
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