Skip to content
This repository has been archived by the owner on May 16, 2018. It is now read-only.

Add "jetbrains-hub" module #1653

Merged
merged 28 commits into from Feb 7, 2017
Merged

Add "jetbrains-hub" module #1653

merged 28 commits into from Feb 7, 2017

Conversation

kris-lab
Copy link
Contributor

@kris-lab kris-lab commented Feb 6, 2017

@kris-lab kris-lab self-assigned this Feb 6, 2017
@kris-lab kris-lab changed the title Add "docker-hub" module Add "jetbrains-hub" module Feb 6, 2017
@kris-lab
Copy link
Contributor Author

kris-lab commented Feb 7, 2017

@kris-lab
Copy link
Contributor Author

kris-lab commented Feb 7, 2017

retest this please

@ppp0
Copy link
Member

ppp0 commented Feb 7, 2017

  • Puppetfile: add helper and daemon
  • Naming: Playing around I felt the dirname /usr/local/hub to be a bit unclear.. how about using /usr/local/jetbrains-hub ?
  • hub.bundle.properties: this installation-uuid=012660ff-7baf-4bfe-a535-85cb7aaf50e4 - how is this generated? Shoudln't a uuid be really "uu" ?
  • install.sh: no need for line 3 and 8? (this is already done by helper::script ?)
  • hub.pp: line 43: only File[$home_path] really required here? File[$var_path] required by daemon in line 59:60 ?
  • hub.pp:
    simplify proxy config? I think the keepalive is not needed (see http://serverfault.com/questions/314147/keep-alive-response-header-for-jetty-web-server)
@@ -57,13 +57,6 @@ class jetbrains::hub (
 
   $upstream_name = 'jetbrains-hub'
 
-  nginx::resource::upstream { $upstream_name:
-    members             => ["localhost:${port}"],
-    upstream_cfg_append => [
-      'keepalive 100;',
-    ],
-  }
-
   nginx::resource::vhost { "${module_name}-https-redirect":
     listen_port         => 80,
     ssl                 => false,
@@ -83,7 +76,7 @@ class jetbrains::hub (
     location_cfg_append => [
       'proxy_set_header Host $http_host;',
       'proxy_set_header X-Forwarded-Proto https;',
-      "proxy_pass http://${upstream_name};",
+      "proxy_pass http://localhost:${port};",
     ],
   }
 }
  • Last, not least: When re-applying puppet, there's always changes:
Puppet applying: `/vagrant/modules/jetbrains/spec/default/manifest.pp`
Notice: Compiled catalog for debian-8-amd64-plain.vagrantup.com in environment production in 1.30 seconds
Notice: /Stage[main]/Jetbrains::Hub/File[/usr/local/hub/conf/internal/bundle.properties]/content: content changed '{md5}464f303b0562baf3d24dcf82f2fc01bf' to '{md5}ab03741e31ad5c06080907c2b79d392e'
Notice: /Stage[main]/Jetbrains::Hub/Daemon[jetbrains-hub]/Service[jetbrains-hub]: Triggered 'refresh' from 1 events

I will re-review later again

@kris-lab
Copy link
Contributor Author

kris-lab commented Feb 7, 2017

hub.bundle.properties: this installation-uuid=012660ff-7baf-4bfe-a535-85cb7aaf50e4 - how is this generated? Shoudln't a uuid be really "uu" ?

This is auto generated by install process or re-used if available in config file. The reason for hardcoding is... to avoid puppet to re-apply every run. I have changed that e65bda7 and this is relevant to your another note "Last, not least: When re-applying puppet, there's always changes"

See e.g. uuid generation thread https://plugins.jetbrains.com/idea/plugin/8320-uuid-generator

@kris-lab
Copy link
Contributor Author

kris-lab commented Feb 7, 2017

@ppp0 please re-review, agree on all points, but installation-uuid=012660ff-7baf-4bfe-a535-85cb7aaf50e4 is a tricky one. Maybe we should move this to the class params? So anyone can adjust the UUID with jetbrains generator?

@ppp0
Copy link
Member

ppp0 commented Feb 7, 2017

  • hub.pp: line 63 to be removed, too (my bad)
  • installation_uuid: how about we use the stdlib function fqdn_uuid and let it generate inside the class? UUID needs to comply with RFC 4122

..may be more..

@kris-lab
Copy link
Contributor Author

kris-lab commented Feb 7, 2017

@ppp0 please re-review

@ppp0
Copy link
Member

ppp0 commented Feb 7, 2017

  • application.pp: The names for nginx::resource::vhost in line 66 and 75 won't be unique like this. Add ${name} as a specifier

@kris-lab
Copy link
Contributor Author

kris-lab commented Feb 7, 2017

@ppp0 please re-review, I have also dropped jetbrains::common.

I think we can ignore the build/specs run for the moment, it runs locally fine, but CI has problems with apt, I think!

@ppp0
Copy link
Member

ppp0 commented Feb 7, 2017

  • CI times out on the install script.. Let's see if it can take it with 20000 secs (!)
  • jetbrains::common 👍
    lgtm

@kris-lab kris-lab merged commit d04a8bd into cargomedia:master Feb 7, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants