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

Feature/14 review shiny ci cd #27

Merged
merged 37 commits into from
Jun 1, 2021
Merged

Conversation

Chargothrond
Copy link
Contributor

@Chargothrond Chargothrond commented Apr 29, 2021

Reviewed and updated shiny ci cd chapter, as well as the github actions yaml examples.
Closes #14.

Copy link
Contributor

@fvitalini fvitalini left a comment

Choose a reason for hiding this comment

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

Some minor reqording. My main request is to clarify better some steps in the yml files by adding some comments.

shiny-ci-cd.Rmd Outdated Show resolved Hide resolved
shiny-ci-cd.Rmd Show resolved Hide resolved
shiny-ci-cd.Rmd Outdated Show resolved Hide resolved
shiny-ci-cd.Rmd Outdated Show resolved Hide resolved
shiny-ci-cd.Rmd Outdated Show resolved Hide resolved
shiny-ci-cd/actions/ci-cd.yml Outdated Show resolved Hide resolved
shiny-ci-cd/actions/ci-cd.yml Show resolved Hide resolved
shiny-ci-cd/actions/ci-cd.yml Outdated Show resolved Hide resolved
shiny-ci-cd/actions/ci-renv.yml Show resolved Hide resolved
shiny-ci-cd/actions/ci.yml Show resolved Hide resolved
Chargothrond and others added 10 commits May 5, 2021 12:21
Co-authored-by: Francesca Vitalini <francesca.vitalini@mirai-solutions.com>
Co-authored-by: Francesca Vitalini <francesca.vitalini@mirai-solutions.com>
Co-authored-by: Francesca Vitalini <francesca.vitalini@mirai-solutions.com>
Co-authored-by: Francesca Vitalini <francesca.vitalini@mirai-solutions.com>
Co-authored-by: Francesca Vitalini <francesca.vitalini@mirai-solutions.com>
Co-authored-by: Francesca Vitalini <francesca.vitalini@mirai-solutions.com>
Co-authored-by: Francesca Vitalini <francesca.vitalini@mirai-solutions.com>
Co-authored-by: Francesca Vitalini <francesca.vitalini@mirai-solutions.com>
@Chargothrond
Copy link
Contributor Author

Thanks for the feedback @fvitalini , integrated most of it apart from the comments regarding the .yml files.

As I mentioned already in miraisolutions/ShinyCICD#1, we need to decide on how much we want to comment these files to explain them better.

@riccardoporreca Please let me know your opinion on this as well, and you can also review the changes now as I integrated Francesca's feedback.

Copy link
Member

@riccardoporreca riccardoporreca left a comment

Choose a reason for hiding this comment

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

See some refinements following the main restructuring with Actions are being the prominent part here. The main gap is talking about renv in the actions section.

.github/workflows/site.yaml Outdated Show resolved Hide resolved
shiny-ci-cd.Rmd Outdated Show resolved Hide resolved
shiny-ci-cd.Rmd Outdated Show resolved Hide resolved
shiny-ci-cd.Rmd Outdated Show resolved Hide resolved
shiny-ci-cd.Rmd Outdated Show resolved Hide resolved
shiny-ci-cd.Rmd Outdated Show resolved Hide resolved
shiny-ci-cd.Rmd Show resolved Hide resolved
shiny-ci-cd.Rmd Show resolved Hide resolved
shiny-ci-cd.Rmd Show resolved Hide resolved
shiny-ci-cd.Rmd Outdated Show resolved Hide resolved
Chargothrond and others added 7 commits May 10, 2021 10:15
Co-authored-by: Riccardo Porreca <riccardo.porreca@mirai-solutions.com>
Co-authored-by: Riccardo Porreca <riccardo.porreca@mirai-solutions.com>
Co-authored-by: Riccardo Porreca <riccardo.porreca@mirai-solutions.com>
Co-authored-by: Riccardo Porreca <riccardo.porreca@mirai-solutions.com>
Co-authored-by: Riccardo Porreca <riccardo.porreca@mirai-solutions.com>
Co-authored-by: Riccardo Porreca <riccardo.porreca@mirai-solutions.com>
Co-authored-by: Riccardo Porreca <riccardo.porreca@mirai-solutions.com>
Chargothrond and others added 12 commits May 10, 2021 10:29
Co-authored-by: Riccardo Porreca <riccardo.porreca@mirai-solutions.com>
Co-authored-by: Riccardo Porreca <riccardo.porreca@mirai-solutions.com>
Co-authored-by: Riccardo Porreca <riccardo.porreca@mirai-solutions.com>
Co-authored-by: Riccardo Porreca <riccardo.porreca@mirai-solutions.com>
* In particular, mention the continuous deployment from `master` only, as in the Travis CI section.
* For the 4 flavors, using the `<details>` HTML tag providing implementing collapsible details.
* Missing in comparison to the Travis CI part, used as a starting point.
* No specific details are included, just pointers to the renv vignette and the complete workflow files.
* Also fix the actual file content shown for each flavor
@riccardoporreca
Copy link
Member

@Chargothrond, see my follow-up updates and other smaller refinements I committed.

We may want to make sure there is full alignment with the workflows as showcased in miraisolutions/ShinyCICD#1

Copy link
Member

@riccardoporreca riccardoporreca left a comment

Choose a reason for hiding this comment

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

We should also delete file .travis.yml, as Travis will not run any more and the focus is now fully on GitHub Actions

@Chargothrond
Copy link
Contributor Author

@riccardoporreca I have aligned the site.yml and deleted the travis.yml, let me know if anything else is required, otherwise you can go ahead with merging.

Copy link
Member

@riccardoporreca riccardoporreca left a comment

Choose a reason for hiding this comment

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

Refined comment about renv and fixed installation of remotes with renv as a final review.

I will commit the suggestions and squash-merge.

shiny-ci-cd/actions/ci-cd-renv.yml Outdated Show resolved Hide resolved
shiny-ci-cd/actions/ci-renv.yml Outdated Show resolved Hide resolved
shiny-ci-cd/actions/ci-cd-renv.yml Show resolved Hide resolved
shiny-ci-cd/actions/ci-renv.yml Show resolved Hide resolved
* Not necessarily Linux-specific
* Mainly there in view of caching what we explicitly define as the renv root
* In general, we there is no guarantee of having remotes installed
@riccardoporreca riccardoporreca self-assigned this Jun 1, 2021
@riccardoporreca riccardoporreca merged commit e872999 into master Jun 1, 2021
@riccardoporreca riccardoporreca deleted the feature/14-review-shiny-ci-cd branch June 1, 2021 15:41
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.

Review Shiny CI/CD
3 participants