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

fix: CONTRIBUTING with project name #53

Merged
merged 1 commit into from
Apr 26, 2022
Merged

fix: CONTRIBUTING with project name #53

merged 1 commit into from
Apr 26, 2022

Conversation

RezaRahemtola
Copy link
Member

@RezaRahemtola RezaRahemtola commented Apr 19, 2022 •
edited
Loading

Description

CONTRIBUTING.md template updated with the project name and repository link
Related to issue #41

Changes include

  • Bugfix (non-breaking change that solves an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (change that is not backwards-compatible and/or changes current functionality)

Checklist

  • I have assigned this PR to myself
  • I have added at least 1 reviewer
  • I have added the needed labels
  • I have linked this PR to an issue
  • I have linked this PR to a milestone
  • I have linked this PR to a project
  • I have tested this code
  • I have added / updated tests (unit / functionals / end-to-end / ...)
  • I have updated the README and other relevant documents (guides...)
  • I have added sufficient documentation both in code, as well as in the READMEs

@RezaRahemtola RezaRahemtola added the documentation Improvements or additions to documentation label Apr 19, 2022
@netlify
Copy link

netlify bot commented Apr 19, 2022 •
edited
Loading

✅ Deploy Preview for nimble-praline-605cf6 ready!

Name Link
🔨 Latest commit f46432a
🔍 Latest deploy log https://app.netlify.com/sites/nimble-praline-605cf6/deploys/625f185ca5d6e9000824e43d
😎 Deploy Preview https://deploy-preview-53--nimble-praline-605cf6.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@RezaRahemtola RezaRahemtola linked an issue Apr 19, 2022 that may be closed by this pull request
[Feature]: Open-Source skills development #41
Closed
8 tasks
adrienfort
adrienfort previously requested changes Apr 20, 2022
View reviewed changes
Copy link
Member

@adrienfort adrienfort left a comment

Choose a reason for hiding this comment

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

LGTM but this PR doesn't respect our workflow : remind that you have first to make a PR from fix/contributing to dev, and then once it's merged, from dev to master.

It's important to strictly follow this workflow, especially at the beginning of the project and even for small changes.

Moreover, please do not remove check points in the PR's checklist. You've removed these two :

 - [ ] I have tested this code
 - [ ] I have added / updated tests (unit / functionals / end-to-end / ...)

Be aware that it's ok if everything is not ticked, sometimes you don't need it. But don't remove them.

Otherwise, you've assigned this PR to the right issue with an assignee, that's great !

@adrienfort adrienfort closed this Apr 20, 2022
@adrienfort adrienfort reopened this Apr 20, 2022
@RezaRahemtola RezaRahemtola changed the base branch from master to dev April 20, 2022 07:46
@RezaRahemtola
Copy link
Member Author

@adrienfort I used your PR #40 that added this template as an example for this one, and as it was merged directly on master (the dev branch currently has an old version of CONTRIBUTING.md ) I thought it was ok to merge it on master.

Same for the checkpoints, you also removed these two in your PR so I did the same.

Anyway it's fixed now !

TomChv
TomChv reviewed Apr 20, 2022
View reviewed changes
Copy link
Member

@TomChv TomChv left a comment

Choose a reason for hiding this comment

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

Can you correctly rebase your PR? I see a bunch of commits from dependabot and merge commit.

git rebase dev (or main) should be enough.
Otherwise, LGTM.

@adrienfort
Copy link
Member

adrienfort commented Apr 20, 2022 •
edited
Loading

@adrienfort I used your PR #40 that added this template as an example for this one, and as it was merged directly on master (the dev branch currently has an old version of CONTRIBUTING.md ) I thought it was ok to merge it on master.

Same for the checkpoints, you also removed these two in your PR so I did the same.

Anyway it's fixed now !

Yes your are right ! I forgot this

@RezaRahemtola
Copy link
Member Author

git rebase dev (or main) should be enough. Otherwise, LGTM.

I tried git rebase origin/master while on the dev branch but I don't have write access to publish the changes:

git push origin dev:dev
remote: error: GH006: Protected branch update failed for refs/heads/dev.
remote: error: At least 1 approving review is required by reviewers with write access.
To github.com:PoCInnovation/InterPlanetaryCloud.git
! [remote rejected] dev -> dev (protected branch hook declined)
error: failed to push some refs to 'github.com:PoCInnovation/InterPlanetaryCloud.git'

@adrienfort
Copy link
Member

git rebase dev (or main) should be enough. Otherwise, LGTM.

I tried git rebase origin/master while on the dev branch but I don't have write access to publish the changes:

git push origin dev:dev
remote: error: GH006: Protected branch update failed for refs/heads/dev.
remote: error: At least 1 approving review is required by reviewers with write access.
To github.com:PoCInnovation/InterPlanetaryCloud.git
! [remote rejected] dev -> dev (protected branch hook declined)
error: failed to push some refs to 'github.com:PoCInnovation/InterPlanetaryCloud.git'

Yes ! That's because the dev branch has protection rules

@RezaRahemtola RezaRahemtola changed the base branch from dev to master April 20, 2022 10:11
@RezaRahemtola RezaRahemtola changed the base branch from master to dev April 20, 2022 10:11
@RezaRahemtola
Copy link
Member Author

Can you correctly rebase your PR? I see a bunch of commits from dependabot and merge commit.

Fixed !

adrienfort
adrienfort approved these changes Apr 20, 2022
View reviewed changes
Copy link
Member

@adrienfort adrienfort left a comment

Choose a reason for hiding this comment

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

LGTM, everything seems clean !

@adrienfort
Copy link
Member

adrienfort commented Apr 20, 2022 •
edited
Loading

Before merging, @RezaRahemtola @TristanMasselot @Samoten777 have you played on your own with PoC's project template ?
As mention in issue #41

@adrienfort adrienfort added the minor Minor update (for release) label Apr 20, 2022
@Samoten777
Copy link
Contributor

Yes, I used it in a personal repository, and it works perfectly fine ! 👍🏾

@adrienfort adrienfort merged commit 61bc62b into dev Apr 26, 2022
@adrienfort adrienfort deleted the fix/contributing branch April 26, 2022 16:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TomChv TomChv TomChv left review comments

@adrienfort adrienfort adrienfort approved these changes

Assignees

@RezaRahemtola RezaRahemtola

@MizuriGit MizuriGit

@Samoten777 Samoten777

Labels
documentation Improvements or additions to documentation minor Minor update (for release)
Projects
None yet
Milestone
Storage - Skills development
Development

Successfully merging this pull request may close these issues.

[Feature]: Open-Source skills development
5 participants
@RezaRahemtola @adrienfort @Samoten777 @TomChv @MizuriGit