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

Websites: Clarify deploy and tag stage #119

Closed
wants to merge 4 commits into from

Conversation

agcolom
Copy link
Member

@agcolom agcolom commented May 3, 2015

Fixes gh-116

`git push --tags origin master`
Afterwards, make sure to push both version change commit and the tag to the main jQuery repo.

<div class="warning">You'll want to check your remote setup using ```git remote -v``` and check which remote is used for the main jQuery repo.</div>
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't render correctly. Check the GitHub preview of this markdown file. From the markdown spec:

Note that Markdown formatting syntax is not processed within block-level HTML tags. E.g., you can’t use Markdown-style emphasis inside an HTML block.

Copy link
Member Author

Choose a reason for hiding this comment

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

@arthurvr It all looks fine for me when deployed locally. Also should I use single ``` instead?

Copy link
Member

Choose a reason for hiding this comment

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

well, this is how it looks on GitHub:
contribute_jquery_org_web-sites_md_at_clarifydeploy_ _agcolom_contribute_jquery_org

If it works fine for our sites that's in fact a buggy behaviour of our markdown parser so I'd prefer to fix it (just use html <code> syntax directly)

Oh and yeah, except for this line, let's be consistent and use a single backtick for inline code instead.

@agcolom
Copy link
Member Author

agcolom commented May 3, 2015

@arthurvr Yes, I think i've fixed it here

@mgol
Copy link
Member

mgol commented May 3, 2015

Looks fine but I have one doubt - are there more places where we describe steps for maintainers that might have the repo set up to have an origin upstream remote? This would be quite repetitive to put this same text in all those places.

@@ -120,8 +120,14 @@ For other sites:

To create a tag, use `npm version [major | minor | patch]`.

Afterwards, make sure to push both version change commit and the tag:
`git push --tags origin master`
Afterwards, make sure to push both version change commit and the tag to the main jQuery repo.
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this change is necessary, especially with the warning included.

@agcolom
Copy link
Member Author

agcolom commented May 3, 2015

@scottgonzalez Thanks for the feedback. I've updated the PR with your comments, and put it in a warning div.
@mzgol I may have missed it but I have never come across such an explanation anywhere on our sites.

@mgol
Copy link
Member

mgol commented May 3, 2015

@mzgol I may have missed it but I have never come across such an explanation anywhere on our sites.

I meant that this notice will make only maintainers that go to this specific site see the notice about upstream/origin, others won't. So for it to be complete it should either be everywhere or nowhere; but "everywhere" will mean either repetition or linking to a common source.

@agcolom
Copy link
Member Author

agcolom commented May 3, 2015

@mzgol oh I see. I suppose we could in that case copy the same warning div to this page: http://contribute.jquery.org/repo-maintainers-guide/ where we first mention pushing to upstream. @scottgonzalez What do you think?

@agcolom
Copy link
Member Author

agcolom commented May 3, 2015

Or we put the warning just before the heading http://local.contribute.jquery.org/repo-maintainers-guide/#fixing-commits (so just after we mention both origin and upstream) and say

The examples above use upstream for the repo in the jQuery organization on GitHub. Use git remote -v to ensure that you're pushing to the correct remote repo. For example, if you use orgin for the repo in the jQuery organization, and ustream for your fork, swap upstream with origin.

@mgol
Copy link
Member

mgol commented May 3, 2015

For example, if you use (..) ustream for your fork.

This seems weird, I doubt many people will name their forks upstream; it's more likely they'll use their own name or sth else. I'd remove that part.

That page already has this:

Assuming you have the jquery repo set as your upstream remote

so maybe here it's already covered. Are there more sites where this would be an issue?

@agcolom
Copy link
Member Author

agcolom commented May 3, 2015

@mzgol indeed, that's at the top of the maintainer's guide page. So I'd agree with you, we don't need to add more on that page. I think all other pages are covered. So I think for now we only need to fix the page I'm taking care of in this PR.

@mgol
Copy link
Member

mgol commented May 3, 2015

I think all other pages are covered. So I think for now we only need to fix the page I'm taking care of in this PR.

LGTM then!

@scottgonzalez
Copy link
Member

@scottgonzalez What do you think?

I think you should do whatever makes sense for the people who need the documentation :-)

@agcolom agcolom closed this in b8f0bd4 May 4, 2015
@agcolom agcolom deleted the clarifyDeploy branch May 4, 2015 19:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

Deploying command is kind of confusing
5 participants