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

Releases md #20

Merged
merged 8 commits into from
Jul 2, 2020
Merged

Releases md #20

merged 8 commits into from
Jul 2, 2020

Conversation

hansvancalster
Copy link
Collaborator

Copy link
Member

@florisvdh florisvdh left a comment

Choose a reason for hiding this comment

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

Nice additions. See some comments below.

In the admin tasks, I think more tasks may be added at the end:

  • make a GitHub release from the general tag
  • check success of publication steps at protocols.inbo.be and at Zenodo (records both source and rendered)

CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
Copy link
Member

@florisvdh florisvdh left a comment

Choose a reason for hiding this comment

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

Some further suggestions and ideas.

README.md Show resolved Hide resolved
RELEASES.md Show resolved Hide resolved
Comment on lines +13 to +15
1. merge the branch to the master and add general and specific tags (see [release model](README.md#release-model)):
1. general tag: `protocols-YYYY.NN`
1. specific tag: `protocol-code-YYYY.NN`
Copy link
Member

@florisvdh florisvdh Jun 11, 2020

Choose a reason for hiding this comment

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

An idea for the protocolhelper package, because this step, and step 1 (version number update) is prone to errors? I.e. one or more functions to:

  • determine and print the next protocols tag
  • determine and print the next protocol-specific tag(s), i.e. based on the occurrence of dev version(s) in the working directory of current checked out branch
  • (optionally:) bump version number of the involved protocol(s) accordingly, and return message(s) on this

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good ideas. I've made an issue of your comment inbo/protocolhelper#27

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done in 65687c6

1. The GitHub protocols repo is setup in such a way that branches that are merged in the master branch will be deleted automatically.


For an _update_ of an existing protocol all steps are the same, except for:
For an **update** of an existing protocol all steps are the same, except for:
Copy link
Member

@florisvdh florisvdh Jun 11, 2020

Choose a reason for hiding this comment

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

A further idea for the protocolhelper package, to start updating an existing protocol? A function that creates and checks out a new branch (protocol-specific), branching off from origin/master (even if one has not checked out master) after fetching origin/master. And also it adds ".dev" to the version number. No auto-committing though, but perhaps staging of the latter. It would just need the protocol code as an input.

Something like:

protocol_code <- "string-by-user" # check if it exists

And then this git workflow, wrapped in system() or mimicked by git2r:

git fetch origin master  # 'origin' could also be generalized to take the first line of the output of 'git remote'
git branch protocol_code origin/master # make the branch
git push -u origin protocol_code:protocol_code # push the branch
git checkout protocol_code # to be put at the end because it is not a fail-safe command (it may error because of unclean working directory)

Plus the adding of .dev (without committing, but perhaps already staged (git add X)).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

idemdito see inbo/protocolhelper#27

Copy link
Collaborator Author

@hansvancalster hansvancalster Sep 8, 2021

Choose a reason for hiding this comment

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

The workflow now mentions the use of checklist::new_branch() (currently only in the renv-setup branch) which deals with most steps (except the final step) listed here. So I think we can just use that, possibly wrapped into a protocolhelper function to include the final step.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds like a good idea!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@ThierryO after checking the code of checklist::new_branch I noticed that https-protocol to talk to GitHub is not supported. So I guess only SSH is supported. That asks quite a lot of confusing setup-steps from often rather inexperienced Rusers who want to contribute to the protocolsource repo. Is this design decision fixed or can https be allowed. I can write an issue or a PR if I understand the code.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

After reading this, it leaves me doubting a little about the preference for SSH.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Several NARA colleagues are working with ssh for the latest two years now. So far, only one intervention needed. Due to somebody with a new computer.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I followed the steps (and even wrote a vignette for checklist with all steps), but when I try checklist::new_branch() (within my local clone of the checklist package I want to make a branch to commit the vignette), it fails with this error:

Error in fetch(repo, "origin", verbose = verbose) : 
  Error in 'git2r_remote_fetch': error authenticating: failed connecting agent 

This happens inside checklist::clean_git(). I have set git remote set-url origin [email protected]:inbo/checklist.git.

I have no clue what I did wrong. On the other hand, I think issues such as these can be avoided by switching git2r functions to the equivalent gert functions which has automatic authentication (and which would also allow both SSH and HTTPS).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Switching for git2r to gert is a major change. Something I cannot do on short notice. I'm willing to accept a PR .

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I might give it a try, but it will also take some time...

@florisvdh
Copy link
Member

I think we forgot to merge this one. Doing that now.

@florisvdh florisvdh merged commit 8ee35fb into master Jul 2, 2020
@florisvdh florisvdh deleted the releases-md branch July 2, 2020 13:27
hansvancalster pushed a commit that referenced this pull request Jan 31, 2023
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