Skip to content

Ensure custom URLs are allowed in use_github_action()#1065

Merged
jennybc merged 10 commits into
r-lib:masterfrom
coatless:fix-gh-actions
Apr 6, 2020
Merged

Ensure custom URLs are allowed in use_github_action()#1065
jennybc merged 10 commits into
r-lib:masterfrom
coatless:fix-gh-actions

Conversation

@coatless
Copy link
Copy Markdown
Contributor

This PR fixes how URLs are setup in use_github_action() by moving the appending of the workflow name outside of the url check.

Note: This is required as the name parameter must be given but isn't used with a custom url.

@hadley hadley requested a review from jimhester March 18, 2020 12:59
@jimhester
Copy link
Copy Markdown
Member

Actually the original intent was that you specify the full url, including the name if you are using a custom url. So I think the way to fix this is to put the custom name logic inside the is.null(url) conditional.

@jennybc
Copy link
Copy Markdown
Member

jennybc commented Mar 27, 2020

@coatless Do you want to take another pass? A usethis release is planned for April 6 (#1076).

@coatless
Copy link
Copy Markdown
Contributor Author

@jennybc sure.

@jimhester So, moving the naming logic in gives:

if (is.null(url)) {
  stopifnot(is_string(name))
  if (!grepl("[.]yaml$", name)) {
    name <- paste0(name, ".yaml")
  }
  url <- glue("https://raw.githubusercontent.com/r-lib/actions/master/examples/{name}")	
}

The function call for a custom action would then be:

use_github_action(url = "https://raw.githubusercontent.com/r-lib/actions/master/examples/blah.yaml")

Is there a reason for not wanting the URL to point to a generic location?

@jimhester
Copy link
Copy Markdown
Member

I am not sure I understand your question, but yes the behavior you describe is the intended usage.

@coatless
Copy link
Copy Markdown
Contributor Author

@jimhester Rephrase:

Given the current function call of:

use_github_action(action)

Wouldn't it make more sense to have it:

use_github_action(action, url="generic")

Instead of:

use_github_action(url="specific/to/action.yaml")

coatless added 2 commits April 1, 2020 21:28
Merge remote-tracking branch 'upstream/master' into fix-gh-actions

# Conflicts:
#	NEWS.md
@coatless
Copy link
Copy Markdown
Contributor Author

coatless commented Apr 2, 2020

@jimhester when you have a moment.

Comment thread R/github-action.R Outdated
@coatless coatless requested a review from jimhester April 2, 2020 17:07
Comment thread NEWS.md Outdated
Comment thread R/github-action.R
coatless and others added 3 commits April 3, 2020 20:08
Co-Authored-By: Jennifer (Jenny) Bryan <jenny.f.bryan@gmail.com>
@coatless coatless requested a review from jennybc April 4, 2020 01:43
Comment thread R/github-action.R
}
# Append a `.yaml` extension if needed
name = gsub("[.]yml$", ".yaml", name)
if (!grepl("[.]yaml$", name)) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If you keep it, please switch to name <- for assignment, instead of name =.

But it feels like it would be simpler to, instead, use regex of [.]ya?ml$ inside the if() (caveat: untested but I assume you see what I mean).

I don't see how we can just substitute .yaml for .yml if that is not the extension used by the workflow file. We can just allow both .yaml and .yml through, yeah?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@jennybc with @jimhester's scoping, the file extension is a pretty specific case to what is listed here:

https://github.com/r-lib/actions/tree/master/examples

So, the following URL would work:

https://raw.githubusercontent.com/r-lib/actions/master/examples/pkgdown.yaml

But, this wouldn't:

https://raw.githubusercontent.com/r-lib/actions/master/examples/pkgdown.yml

As a result, I think a two-pass solution is only viable, where the first pass removes replaces .yml with .yaml if present and the second pass verifies the extension is present.

Sorry, about the = assignment.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't think this extra logic is necessary, the examples in r-lib/actions (which is the only place that the 'name' parameter will be used) are always going be .yaml.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I would revert df603d0

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Okay, reverted. Thanks @jimhester !

Comment thread tests/testthat/test-github-action.R Outdated

})

test_that("use_github_action() replaces yml with yaml in name", {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If I'm right above, I think this can be added as a second use_github_action() call in the test above, with .yml instead of .yaml.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The .yml / .yaml components would only be enforced when there isn't a custom URL. So, I don't think this can be merged in with the above test as it is checking:

use_github_action("pkgdown.yml")

whereas the first test is ensuring that a custom URL can be pulled and downloaded.

use_github_action(url="https://my-secret/gh/actions/master/examples/pkgdown.yaml")

Comment thread tests/testthat/test-github-action.R Outdated
@jennybc
Copy link
Copy Markdown
Member

jennybc commented Apr 4, 2020

@jimhester can you take a quick look here? If you 👍 before Monday-ish, it will be in the release.

coatless and others added 3 commits April 4, 2020 10:59
Co-Authored-By: Jennifer (Jenny) Bryan <jenny.f.bryan@gmail.com>
@coatless
Copy link
Copy Markdown
Contributor Author

coatless commented Apr 4, 2020

@jennybc should be good to go now once the CI finishes :)

@coatless
Copy link
Copy Markdown
Contributor Author

coatless commented Apr 5, 2020

@jennybc this passes cleanly. For whatever reason, the macOS-latest build was cancelled.

@jennybc
Copy link
Copy Markdown
Member

jennybc commented Apr 6, 2020

Thanks all!

@jennybc jennybc merged commit 4d47139 into r-lib:master Apr 6, 2020
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