Skip to content

fix testing instructions in CONTRIBUTING.md #6305

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
Merged
merged 6 commits into from
Mar 18, 2019
Merged

fix testing instructions in CONTRIBUTING.md #6305

merged 6 commits into from
Mar 18, 2019

Conversation

stevegt
Copy link
Contributor

@stevegt stevegt commented Mar 11, 2019 •
edited
Loading

@codecov-io
Copy link

codecov-io commented Mar 11, 2019 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (master@1155f1b). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #6305   +/-   ##
=========================================
  Coverage          ?   38.87%           
=========================================
  Files             ?      363           
  Lines             ?    51210           
  Branches          ?        0           
=========================================
  Hits              ?    19910           
  Misses            ?    28432           
  Partials          ?     2868

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1155f1b...80e7aff. Read the comment docs.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Mar 11, 2019
This was referenced Mar 11, 2019
Closed
Closed
@lunny
Copy link
Member

lunny commented Mar 17, 2019

I think maybe many of us are using macOS?or windows?

@stevegt
Copy link
Contributor Author

stevegt commented Mar 17, 2019

@lunny I finally started bringing myself up to speed on go this past week; am thinking of converting this script from bash to go so it will be more portable. Should we kill this PR, or keep it open and I update it with the go version?

@stevegt
Copy link
Contributor Author

stevegt commented Mar 17, 2019

On the other hand, I worded the changes to CONTRIBUTING.md such that if you're on non-Linux you would still install drone-cli manually just like now; for manual installation I'm adding the info about disk space and drone-cli version. This means we have the option of accepting this PR as-is; it would be better than what we have in CONTRIBUTING.md right now. (Right now the testing instructions in CONTRIBUTING.md don't work at all, due to the release of drone-cli 1.x.)

I do think that the script should be in go rather than bash for portability though, and if we were to accept this PR now I think the script should be replaced with a go version later.

@stevegt
Copy link
Contributor Author

stevegt commented Mar 17, 2019

I've removed the idea of automated drone run at PR submission from both this PR and from #6269. Still thinking. Maybe the right thing to do with this PR is to remove the script itself and remove mention of the script from CONTRIBUTING.md, leaving in there the new instructions about disk space and drone version. That will fix #6243.

@stevegt
Copy link
Contributor Author

stevegt commented Mar 17, 2019

Okay, I've removed the bash script and left in place an updated set of run instructions in CONTRIBUTING.md. This fixes #6243.

Until we have the go version, I'll leave the bash script at https://github.com/stevegt/gitea/blob/drone-cli-bash/scripts/test-local.sh for reference.

@stevegt stevegt changed the title add scripts/test-local.sh and fix CONTRIBUTING.md fix testing instructions in CONTRIBUTING.md Mar 17, 2019
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Mar 17, 2019
@techknowlogick techknowlogick added the type/docs This PR mainly updates/creates documentation label Mar 17, 2019
adelowo
adelowo approved these changes Mar 17, 2019
View reviewed changes
Copy link
Member

@adelowo adelowo left a comment

Choose a reason for hiding this comment

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

👏👏

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Mar 17, 2019
@lunny lunny merged commit cd8cdbd into go-gitea:master Mar 18, 2019
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@techknowlogick techknowlogick techknowlogick approved these changes

@adelowo adelowo adelowo approved these changes

Assignees
No one assigned
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/docs This PR mainly updates/creates documentation type/testing
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Testing instructions in CONTRIBUTING.md need to include drone-cli version and disk space requirements
6 participants
@stevegt @codecov-io @lunny @techknowlogick @adelowo @GiteaBot