Skip to content
This repository was archived by the owner on Jul 24, 2020. It is now read-only.

501 party foul #770

Merged
merged 8 commits into from
Sep 11, 2014
Merged

501 party foul #770

merged 8 commits into from
Sep 11, 2014

Conversation

orenyk
Copy link
Contributor

@orenyk orenyk commented Jul 22, 2014

Resolves #501, see #750

@dgoerger
Copy link
Contributor

What's the status on this? Is it ready to be reviewed/merged? Does it do any harm if there's no OAuth key? (i.e., will it break anything or spit out errors if we have this code in master without a key?)

@orenyk
Copy link
Contributor Author

orenyk commented Jul 23, 2014

My guess is that it shouldn't break anything if we merge, but I'm not sure about the OAuth situation. I'll look over the deploy script and see when this is called and find out what happens if the key is missing.

@dgoerger
Copy link
Contributor

bump?

@orenyk
Copy link
Contributor Author

orenyk commented Jul 31, 2014

I haven't had a chance to look into this; I don't think this will go in until v4.1 so it's on hold for the moment.

@orenyk
Copy link
Contributor Author

orenyk commented Sep 7, 2014

I've merged in master and deleted the config/deploy.rb file that is left over from the days when we used Capistrano for deployment and is no longer used. I'm going to set up an example initializer file without the OAuth token that we'll modify using our deploy script.

@orenyk
Copy link
Contributor Author

orenyk commented Sep 10, 2014

Ok, this is all but ready to go. I've got to ask Mike to set up the renaming of the initializer and copying of the OAuth token in the deploy script, but this should be done by the time we want to deploy v4.0.

EDIT I wouldn't mind if someone gave this a final look through to make sure it all seems to make sense. Thanks!

@mnquintana
Copy link
Contributor

Hmm, did you find out why we don't use Capistrano anymore? I read Mike's install script for Reservations, and it's a completely custom-rolled shell script instead of a standard like Capistrano, which sort of concerns me. Not to mention, I feel like we need to keep Capistrano deployment open for non-Yale users - removing that deploy script is basically robbing them of that ability and turning this into a Yale-only project again.

@dgoerger what do you think?

@orenyk
Copy link
Contributor Author

orenyk commented Sep 10, 2014

I'm not sure when we stopped using Capistrano and moved to Jenkins, the script was last updated in March of 2013 but our setup / deployment procedure has changed since then and no one has been updating the deploy.rb script and maintaining two separate deployment scripts is not something we have the manpower for right now. We can leave the file in if you think that's better, but I feel like having an outdated file is almost worse than having no file. We should have Heroku deployments working in the next month or two (see #275), and properly documenting our deployment procedures is high up on the list of priorities (#683). Once that's done we may want to define some deployment script for Capistrano or a similar mechanism, or we may leave that to the sysadmins at any future client who will know their needs and infrastructure better than we do.

@mnquintana
Copy link
Contributor

Reservations really shouldn't require a sysadmin to deploy (and shouldn't otherwise require Heroku, although it is awesome that we're adding that).

I think it's definitely worth either investigating better general deployment strategies for non-Yale users (maybe there's something better than Capistrano now?), writing a more generic Jenkins script and prescribing that non-Yale users use Jenkins for deployment (seems like a tall order because it requires hosting your own CI server), or updating the Capistrano deploy script to work for anyone.

Although I'm just speculating, my guess is that we stopped using Capistrano because our deployment people in INF aren't Rubyists, hence, shell scripts. But we are, and we should be able to maintain our own deploy scripts.

@orenyk
Copy link
Contributor Author

orenyk commented Sep 10, 2014

Fair point, and I agree that we should provide some kind of generalized deployment setup for future clients. I'll just reiterate that given our current time constraints and manpower, figuring out and setting up a new deployment script is not at the top of the list, and the current script is outdated and should be removed as such. The nice thing about source control is I can still link to the file in the deployment issue (#683) even if I remove it from master for now, so we don't lose whatever knowledge was built into it and can use it as a basis for future work but don't leave a broken file in our repo. Again, I definitely agree with your point in general, but I'd rather have no script than a broken one.

@dgoerger
Copy link
Contributor

Although I'm just speculating, my guess is that we stopped using Capistrano because our deployment people in INF aren't Rubyists, hence, shell scripts.

I think this is probably a fair assessment. A quick Internet search seems to suggest Jenkins + Capistrano is quite doable. I was recently (three hours ago) given a lot more Jenkins responsibility within ITS, so we might actually be able to make this work... we'll have to test it. The code should ideally be easily reusable outside of ITS, or if ITS ever moves away from Jenkins, so I think I could sell this change as long as there aren't any hidden downsides---I'll need to confer with Mike and Jessica. Change (if approved) will take time tho.

I agree however that a broken script is more irritating than no script at all, especially since we can go back in the history and recover the script at resurrection time. Alternatively.. we could keep the script but put in big comments at the top DON'T BE SURPRISED IF THIS DOESN'T WORK, WE'RE WORKING ON FIXING IT PLZ SEE ISSUE HHH ?

@orenyk
Copy link
Contributor Author

orenyk commented Sep 11, 2014

That's great news, we'll definitely put together a proper Capistrano deployment script in #683. I'd rather take the broken script out, and since this whole discussion is separate from the issue at hand (e.g. party foul) I think I can merge this in and we'll continue the discussion in the other issue. Thanks guys!

orenyk added a commit that referenced this pull request Sep 11, 2014
@orenyk orenyk merged commit 5088873 into master Sep 11, 2014
@caseywatts
Copy link
Collaborator

@orenyk @dgoerger @mnquintana you guys are my heroes. This all makes so much sense, I'm sooo glad we're having this conversation! :D

@orenyk orenyk deleted the 501_party_foul branch October 27, 2014 15:31
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.

Replace Airbrake with Party Foul
4 participants