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

Combine update combined_metadata and data-raw actions #208

Merged
merged 1 commit into from
Apr 23, 2021

Conversation

jwokaty
Copy link
Contributor

@jwokaty jwokaty commented Apr 21, 2021

This PR combines the actions to update combined_metadata and data-raw to make update-curation.yml. Note that .github/workflows/data-raw.yml is removed and that the curation file is still called sampleMetadata.rda, although it may be changed back to combined_metadata.rda via #196. The action uses cMDC's checkCuration, which is currently failing because it expects column "dataset_name" but has "studyName".

Closes #200.

@jwokaty jwokaty requested review from lwaldron and schifferl April 21, 2021 23:37
@jwokaty jwokaty marked this pull request as draft April 21, 2021 23:37
@schifferl schifferl self-assigned this Apr 22, 2021
Copy link
Collaborator

@schifferl schifferl left a comment

Choose a reason for hiding this comment

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

Why are we doing the Test curation step? Doesn't this happen in the curation repo? I think we need to remove the step and depend on the metadata being correct from inst/curated in the curation repo.

@lwaldron
Copy link
Member

curatedMetagenomicDataCuration provides a vignette and badge to help curators fix errors, but it provides no guarantees of validity of metadata - curators can and do commit preliminary work then fix it sometime later. There is the additional issue that by the time the PR is reviewed here, the curation repo may have been changed further, but still manual review of the PR is desirable because there are limitations of the automatic checks that we sometimes over-ride by either accepting language that fails, or rejecting language that passes (for example see waldronlab/curatedMetagenomicDataCuration#2). To have an assurance of valid metadata in curatedMetagenomicData, tests have to be done in its own workflow.

@jwokaty
Copy link
Contributor Author

jwokaty commented Apr 23, 2021

If and after this is approved, I'd also like to merge this myself so I can rebase it on master when it has the changes for #212 and #211 so I can handle the merge conflicts. This will make for a cleaner history.

Copy link
Collaborator

@schifferl schifferl left a comment

Choose a reason for hiding this comment

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

Approved, but we should probably encapsulate the checking logic in a script rather than the action itself in the future.

@schifferl schifferl marked this pull request as ready for review April 23, 2021 23:32
@schifferl schifferl merged commit 2421d2b into waldronlab:master Apr 23, 2021
@lwaldron
Copy link
Member

we should probably encapsulate the checking logic in a script rather than the action itself in the future.

Which part of the logic? The check itself already relies on the function curatedMetagenomicDataCuration::checkCuration(), this action is about running the check weekly and creating a badge and automatic pull request.

@jwokaty jwokaty deleted the combine-actions branch May 5, 2021 19:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

restore automated PR GitHub Action
3 participants