Add the missing kredits-github::nginx recipe for barnard #53

2019-05-02T09:35:03Z

greg commented

2019-05-02 09:35:03 +00:00

This was causing the firewall rules for ports 80 and 443 to be deleted

Confirmed to work on barnard, can be merged

Refs #35

This was causing the firewall rules for ports 80 and 443 to be deleted Confirmed to work on barnard, can be merged Refs #35

raucao commented

2019-05-02 09:38:54 +00:00

IIRC I designed the cookbook intentionally so that only one recipe is needed. Did you change something about that before/when merging?

raucao commented

2019-05-02 09:46:52 +00:00

By the way, the title is wrong. This has nothing to do with kredits-web.

greg commented

2019-05-02 10:40:11 +00:00

IIRC I designed the cookbook intentionally so that only one recipe is needed. Did you change something about that before/when merging?

Yes, the design couldn't work, the nginx recipe was running before deploying the app, and the nodejs Systemd unit depended on nginx (fabbe398a2). I found my comments, they were at https://gitea.kosmos.org/kosmos/chef/pulls/37/files#issuecomment-303 and https://gitea.kosmos.org/kosmos/chef/pulls/37/files#issuecomment-304 (Gitea's parsing fucks these up, so I turned them into code snippets...) Looking at it again I'm changing the run list of barnard to use the role I created

By the way, the title is wrong. This has nothing to do with kredits-web.

It has everything to do with kredits-web, since it's the only recipe on barnard that depends on nginx and includes the firewall rules for it defined in kosmos-nginx::default

> IIRC I designed the cookbook intentionally so that only one recipe is needed. Did you change something about that before/when merging? Yes, the design couldn't work, the nginx recipe was running before deploying the app, and the nodejs Systemd unit depended on nginx (fabbe398a2e50aa9aa459e27a1327053992187f6). I found my comments, they were at `https://gitea.kosmos.org/kosmos/chef/pulls/37/files#issuecomment-303` and `https://gitea.kosmos.org/kosmos/chef/pulls/37/files#issuecomment-304` (Gitea's parsing fucks these up, so I turned them into code snippets...) Looking at it again I'm changing the run list of barnard to use the role I created > By the way, the title is wrong. This has nothing to do with kredits-web. It has everything to do with kredits-web, since it's the only recipe on barnard that depends on nginx and includes the firewall rules for it defined in `kosmos-nginx::default`

raucao commented

2019-05-02 10:43:17 +00:00

Yes, the design couldn’t work

That doesn't make sense. It's exactly what I ran to successfully configure everything on barnard. It worked just fine before you changed it, as proven by the GitHub app having worked since the day I finished the setup.

It has everything to do with kredits-web

kredits-web is a client-side JS app running on 5apps Deploy. This is a cookbook that configures kredits-github, a node.js app running on a DO box.

> Yes, the design couldn’t work That doesn't make sense. It's exactly what I ran to successfully configure everything on barnard. It worked just fine before you changed it, as proven by the GitHub app having worked since the day I finished the setup. > It has everything to do with kredits-web `kredits-web` is a client-side JS app running on 5apps Deploy. This is a cookbook that configures `kredits-github`, a node.js app running on a DO box.

greg changed title from ~~Add the missing kredits-web::nginx recipe for barnard~~ to Add the missing kredits-github::nginx recipe for barnard

2019-05-02 11:23:07 +00:00

greg commented

2019-05-02 11:42:10 +00:00

Got it, the kredits-web/github mixup was a brainfart on my end. I fixed the title, thanks

Re: the cookbook, I took another look and I was wrong, the issue was actually the Let's Encrypt setup being included in development, that's what prevented the cookbook to work when I ran it in a VM (ecf5870195).

I still think that the default recipe shouldn't include the nginx recipe since they're different responsabilities, but I fucked up by not replacing the recipe in the run list with the role that adds both recipes. I think in this case it would be easier to understand if these recipes had tests, then one would clearly be about deploying the app and starting it as a service, and another one about setting up an nginx reverse proxy in front of it

Got it, the kredits-web/github mixup was a brainfart on my end. I fixed the title, thanks Re: the cookbook, I took another look and I was wrong, the issue was actually the Let's Encrypt setup being included in development, that's what prevented the cookbook to work when I ran it in a VM (ecf5870195669a2e6d01eeaa4b93c215e77cdc79). I still think that the default recipe shouldn't include the nginx recipe since they're different responsabilities, but I fucked up by not replacing the recipe in the run list with the role that adds both recipes. I think in this case it would be easier to understand if these recipes had tests, then one would clearly be about deploying the app and starting it as a service, and another one about setting up an nginx reverse proxy in front of it

raucao commented

2019-05-02 13:47:24 +00:00

Re: the cookbook, I took another look and I was wrong, the issue was actually the Let’s Encrypt setup being included in development, that’s what prevented the cookbook to work when I ran it in a VM

Why isn't that solved by the LE recipe to begin with? Isn't that what Chef environments are for?

I think in this case it would be easier to understand if these recipes had tests, then one would clearly be about deploying the app and starting it as a service, and another one about setting up an nginx reverse proxy in front of it

I don't see how that changes anything. Also, generic node apps like that shouldn't require any special tests, because they should almost exclusively use DSLs from our other cookbooks that should be tested already. "Deploy node process xyz running on local port N with external domain xyz.com" shouldn't even require as much code as was necessary for this one imo.

> Re: the cookbook, I took another look and I was wrong, the issue was actually the Let’s Encrypt setup being included in development, that’s what prevented the cookbook to work when I ran it in a VM Why isn't that solved by the LE recipe to begin with? Isn't that what Chef environments are for? > I think in this case it would be easier to understand if these recipes had tests, then one would clearly be about deploying the app and starting it as a service, and another one about setting up an nginx reverse proxy in front of it I don't see how that changes anything. Also, generic node apps like that shouldn't require any special tests, because they should almost exclusively use DSLs from our other cookbooks that should be tested already. "Deploy node process xyz running on local port N with external domain xyz.com" shouldn't even require as much code as was necessary for this one imo.

greg closed this pull request

2019-05-07 11:48:19 +00:00

greg deleted branch bugfix/35-kredits-github_nginx

2019-05-07 11:48:26 +00:00

Sign in to join this conversation.

2 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: kosmos/chef#53