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

Clarify GitHub webhook configuration #1660

Merged

Conversation

wgordon17
Copy link
Contributor

Signed-off-by: Will Gordon wgordon@redhat.com

@wgordon17
Copy link
Contributor Author

I found the instructions for adding a GitHub webhook to be lacking in this information, and only figured it out after trial and error. I believe that this additional information to be beneficial for users.

Copy link
Member

@spadgett spadgett left a comment

Choose a reason for hiding this comment

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

Thanks @wgordon17, definitely good to clarify this. Can you run grunt build to create the dist/ files and add them to your commit?

</span>
<span ng-if="!($ctrl.createdBuildConfig.spec.source.git.uri | isGithubLink)">
Your source does not appear to be a URL to a GitHub repository. If you have a GitHub repository that you want to trigger this build from then use the following payload URL:
Your source does not appear to be a URL to a GitHub repository. If you have a GitHub repository that you want to trigger this build from then use the following payload URL and specifying a <i>Content type</i> of <mark>application/json</mark>:
Copy link
Member

Choose a reason for hiding this comment

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

We usually use code tags for things like <code>application/json</code>.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, that sounds reasonable. My thinking was since it wasn't actually code, I didn't want to confuse end users =)

@spadgett spadgett self-assigned this Jun 7, 2017
@wgordon17 wgordon17 force-pushed the provide-clearer-hook-instructions branch from 837ea0f to 0f082ee Compare June 8, 2017 13:39
app/index.html Outdated
<script src="bower_components/objectpath/lib/ObjectPath.js"></script>
<script src="bower_components/angular-schema-form/dist/schema-form.js"></script>
<script src="bower_components/angular-schema-form/dist/bootstrap-decorator.js"></script>
<script src="bower_components/angular-schema-form-bootstrap/bootstrap-decorator.min.js"></script>
Copy link
Member

Choose a reason for hiding this comment

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

Looks like you have some diffs that shouldn't be here. Can you stop grunt serve and try

$ git checkout master app/index.html
$ bower install
$ grunt build

Then squash your commits?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought it was odd as well. I didn't touch index.html, the changes came after running grunt build, so I assumed they were expected =P

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, your dependencies are out of date, which bower install will fix

@wgordon17 wgordon17 force-pushed the provide-clearer-hook-instructions branch from 0f082ee to df3eed5 Compare June 8, 2017 13:53
@@ -8062,7 +8062,7 @@ delete c[b.id], delete d[a];
http:80,
https:443,
ftp:21
}, sf = b("$location"), tf = /^\s*[\\/]{2,}/, uf = {
}, sf = b("$location"), tf = /^\s*[\\\/]{2,}/, uf = {
Copy link
Member

Choose a reason for hiding this comment

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

This also looks incorrect :/ What node version do you have? Unfortunately anything newer than Node 4 generates slightly different dist output, which will cause merges to fail.

Probably easiest for now is to

$ git checkout master dist/scripts/vendor.js

since you only have markup changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was the easiest way, but I decided to go the "right" way and downgraded from Node8. Did I miss somewhere in the README about <= Node4 being a requirement?

Copy link
Member

Choose a reason for hiding this comment

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

No, do you mind opening an issue? We either need to update the README or figure out what causes the diff with newer Node versions and fix it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Understood, thanks! =)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done #1667

Signed-off-by: Will Gordon <wgordon@redhat.com>
@wgordon17 wgordon17 force-pushed the provide-clearer-hook-instructions branch from df3eed5 to 800920a Compare June 8, 2017 14:15
@spadgett
Copy link
Member

spadgett commented Jun 8, 2017

[merge]

@openshift-bot
Copy link

Evaluated for origin web console merge up to 800920a

@openshift-bot
Copy link

openshift-bot commented Jun 8, 2017

Origin Web Console Merge Results: SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin_web_console/1486/) (Base Commit: f70768f)

@openshift-bot openshift-bot merged commit 7a49dad into openshift:master Jun 8, 2017
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.

3 participants