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

Validate empty repo #33

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Validate empty repo #33

wants to merge 6 commits into from

Conversation

smashwilson
Copy link
Collaborator

This addresses #23 by:

  • Adding validation to the Project model.
  • Fixing a bug in ProjectsController that only shows up when validation fails 😉
  • Reporting the output of git fetch or git 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.

Ash Wilson added 4 commits January 17, 2014 14:12
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!
@smashwilson
Copy link
Collaborator Author

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 if in there to only show the full backtrace when:

  1. The exception inherits some set of known exceptions that we "expect" to show up, and
  2. You're not running in RAILS_ENV=production.

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 @logger instead? In any case, that's definitely a separate issue.

@@ -1,6 +1,8 @@
class Project < ActiveRecord::Base
has_many :builds, :dependent => :destroy

validates :repo, presence: true
Copy link
Owner

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done. ⚡

@cobyism
Copy link
Owner

cobyism commented Jan 20, 2014

👍 Awesome!

I also prevented git from holding up Sidekiq by asking for a password on stdin while I was at it

💗 I hadn’t run into that myself, but it’s great to have that sorted 😀

Think I should mess with the CSS on this PR too?

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 :bowtie:

When the build fails because of an exception, the stack trace pushes all of the nice messages off into the abyss of scrollback!

Another option might be to add a field/column to the Build model to separately capture and store any stacktraces that happen during a build? That way, the build output could simply say that it ran into an error, and we could display a separate output field below (maybe hidden/collapsed by default) containing the error stacktrace? I dunno… that could be a terrible idea for other reasons too. 😆 Ideally it’d be cool if you could do "folding" in the output directly, but that seems like it’d take a fair bit of effort and time to even see if it’s viable.

@smashwilson
Copy link
Collaborator Author

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 :bowtie:

Sounds good to me! I might take a stab at it at some point if I'm feeling particularly artistic. 🎨

Another option might be to add a field/column to the Build model to separately capture and store any stacktraces that happen during a build? That way, the build output could simply say that it ran into an error, and we could display a separate output field below (maybe hidden/collapsed by default) containing the error stacktrace? I dunno… that could be a terrible idea for other reasons too. 😆 Ideally it’d be cool if you could do "folding" in the output directly, but that seems like it’d take a fair bit of effort and time to even see if it’s viable.

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 BranchNotFoundError "friendly" text explaining details and possible fixes, and show the full stack in a collapsed control.

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 bundle install output and fold it up by default, Travis-style.

Anyway, this is all pretty speculative. I'll pop open a separate issue to improve exception handling, maybe.

@cobyism
Copy link
Owner

cobyism commented Jan 21, 2014

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!

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.

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