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.

Sign up for GitHub

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

Jump to bottom

Contribution guide to document actual guide for pull requests #378

Closed
wants to merge 6 commits into from
Closed

Contribution guide to document actual guide for pull requests #378

wants to merge 6 commits into from

Conversation

khalidmammadov
Copy link
Contributor

@khalidmammadov khalidmammadov commented Jan 30, 2022 •
edited
Loading

Currently contribution guide does not reflect actual flow to raise a new PR and hence it's not clear (for a new contributors) what exactly needs to be done to make a PR for Spark repository and test it as per expectation. This PR addresses that by following:

  • It describes in the Pull request section of the Contributing page the actual procedure and takes a contributor through a step by step process.
  • It removes optional "Running tests in your forked repository" section on Developer Tools page which is obsolete now and doesn't reflect reality anymore i.e. it says we can test by clicking “Run workflow” button which is not available anymore as workflow does not use "workflow_dispatch" event trigger anymore and was removed in [SPARK-35048][INFRA] Distribute GitHub Actions workflows to fork repositories to share the resources spark#32092
  • Instead it documents the new procedure that above PR introduced i.e. contributors needs to use their own GitHub free workflow credits to test new changes they are purposing and a Spark Actions workflow will expect that to be completed before marking PR to be ready for a review.
  • Some general wording was copied from "Running tests in your forked repository" section on Developer Tools page but main content was rewritten to meet objective
  • Also fixed URL to developer-tools.html to be resolved by parser (that converted it into relative URI) instead of using hard coded absolute URL.

Tested imperically with bundle exec jekyll serve and static files were generated with bundle exec jekyll build commands

This closes https://issues.apache.org/jira/browse/SPARK-37996

srowen
srowen requested changes Jan 30, 2022
View reviewed changes
contributing.md Outdated

1. <a href="https://help.github.com/articles/fork-a-repo/">Fork</a> the GitHub repository at
<a href="https://github.com/apache/spark">https://github.com/apache/spark</a> if you haven't already
1. Clone your fork, create a new branch, push commits to the branch.
1. Consider whether documentation or tests need to be added or updated as part of the change,
2. Go to "Actions" tab on your forked repository and enable "Build and test" and "Report test results" workflows
Copy link
Member

Choose a reason for hiding this comment

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

Just write 1. on each of these and revert subsequent changes. The point is that markdown generates the correct numbering

Copy link
Member

Choose a reason for hiding this comment

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

+1 for Sean's comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

contributing.md Show resolved Hide resolved
contributing.md Outdated
tested with a comment like "Jenkins, add to whitelist"
1. After about 2 hours, Jenkins will post the results of the test to the pull request, along
10. The Jenkins automatic pull request builder will test your changes
1. If it is your first contribution, Jenkins will wait for confirmation before building
Copy link
Member

Choose a reason for hiding this comment

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

Revert these indent 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.

done

developer-tools.md Show resolved Hide resolved
dongjoon-hyun
dongjoon-hyun reviewed Feb 1, 2022
View reviewed changes
Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

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

This looks like mostly straight-forward moving the content from developer-tools page to contributing page. Could you make it clear in the PR title and description about what was copied and what was newly revised and added, @khalidmammadov ?

If you don't mind, please update the corresponding JIRA too.

@srowen
Copy link
Member

srowen commented Feb 2, 2022

A little more detail in the description would be nice; we don't need a JIRA for this website change I think

@dongjoon-hyun
Copy link
Member

dongjoon-hyun commented Feb 2, 2022 •
edited
Loading

A little more detail in the description would be nice; we don't need a JIRA for this website change I think

To @srowen: Yes, I agree with you that we don't need a JIRA for this website change. However, @khalidmammadov already filed SPARK-37996 on January 24th as he described in the AS-IS PR description. That's the reason why I asked him to update it.

HyukjinKwon
HyukjinKwon approved these changes Feb 3, 2022
View reviewed changes
Copy link
Member

@HyukjinKwon HyukjinKwon left a comment

Choose a reason for hiding this comment

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

Yeah, updating PR / JIRA descriptions would be great. Otherwise, it looks fine to me 2/

@khalidmammadov
Copy link
Contributor Author

I think all done, anything left to do here?

@srowen srowen closed this in 991df19 Feb 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dongjoon-hyun dongjoon-hyun dongjoon-hyun left review comments

@srowen srowen srowen approved these changes

@HyukjinKwon HyukjinKwon HyukjinKwon approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@khalidmammadov @srowen @dongjoon-hyun @HyukjinKwon