-
Notifications
You must be signed in to change notification settings - Fork 10
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
Validate empty repo #33
base: master
Are you sure you want to change the base?
Conversation
GitHub auth tokens should go in `.env`. (Now to give better feedback if the fetch fails...)
It shows both the attempted command and the captured output if either fail.
If validation fails (which it totally can now), the `new` template gets rendered, which needs `@hosts` set to something reasonable. Whoops!
The validation errors looks kind of ugly in the form. Think I should mess with the CSS on this PR too? Oh, something else. When the build fails because of an exception, the stack trace pushes all of the nice messages off into the abyss of scrollback! I was thinking of adding an
Of course the danger is that we'd make it harder to track down issues if people hit bugs in DCIY itself. Maybe we could log the full trace to |
@@ -1,6 +1,8 @@ | |||
class Project < ActiveRecord::Base | |||
has_many :builds, :dependent => :destroy | |||
|
|||
validates :repo, presence: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it worth adding a regex here to validate that the format is in name-with-owner (i.e. owner/name
) form?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. ⚡
👍 Awesome!
💗 I hadn’t run into that myself, but it’s great to have that sorted 😀
Only if you want to. I wouldn’t worry about merging this in even if the errors look horrible. It’s not that common a problem, and if someone using this is bothered enough about the errors not being pretty, they might be motivated enough to improve them themselves
Another option might be to add a field/column to the |
Sounds good to me! I might take a stab at it at some point if I'm feeling particularly artistic. 🎨
Oh, interesting! I like that as a way of dealing with "internal" problems. Keeps the build output clean, and lets us differentiate between DCIY problems and actual build problems at the same time. We could give exceptions like Folding would be a lot of work, yeah; to get there we'd need to implement build output processing that can recognize what a stack trace looks like. It would be neat (if rather un-minimalistic) to be able to parse out RSpec, Test::Unit, or other testing frameworks' failure output and have special highlighting for the error messages. If you're looking at build output, that's what you're most interested in, after all! A logical extension is to recognize things like Anyway, this is all pretty speculative. I'll pop open a separate issue to improve exception handling, maybe. |
Yeah, that’d be 🤘—also, getting color support for build output would be pretty helpful too, in terms of highlighting what people care about while letting everything else fade into the background. |
This addresses #23 by:
Project
model.ProjectsController
that only shows up when validation fails 😉git fetch
orgit clone
when either fail, and failing the build then too.I also prevented
git
from holding up Sidekiq by asking for a password on stdin while I was at it, since I did some testing against private repos on GitHub.