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

Support '/' in Github Workflows name #4559

Closed
2 of 3 tasks
bagbyte opened this issue Jan 19, 2020 · 24 comments
Closed
2 of 3 tasks

Support '/' in Github Workflows name #4559

bagbyte opened this issue Jan 19, 2020 · 24 comments
Assignees
Labels
bug Bugs in badges and the frontend service-badge Accepted and actionable changes, features, and bugs

Comments

@bagbyte
Copy link

bagbyte commented Jan 19, 2020

Are you experiencing an issue with...

🪲 Description

I'm having an issue using the /github/workflow/status/:user/:repo/:workflow API. My workflow is called Deploy @myorg/lib, so the final URL looks like: /github/workflow/status/myorg/myrepo/Deploy%20%40myorg%2Flib, but this is interpreted by the service as:

{
  user: 'myorg',
  repo: 'myrepo'
  workflow: 'Deploy @myorg'
  branch: 'lib', 
}

It seems the URL is decoded before evaluating the route pattern, causing a general issue that may impact any service path.

Note: This workflow name is supported by GitHub, the official documentation for the badge accepts the encoded workflow name.

@bagbyte bagbyte added the question Support questions, usage questions, unconfirmed bugs, discussions, ideas label Jan 19, 2020
@calebcartwright
Copy link
Member

calebcartwright commented Jan 19, 2020

Can you share the actual badge routes (org and repo name) you are using so that we can debug and test?

Also what's the badge you are currently seeing from Shields? Are you getting Not Found, something else?

@bagbyte
Copy link
Author

bagbyte commented Jan 19, 2020

I've set up a testing repository with a single workflow named Deploy @bagbyte/lib.

@calebcartwright calebcartwright changed the title Support special chars in URL for Github Workflows name Support '/' in Github Workflows name Jan 22, 2020
@calebcartwright
Copy link
Member

calebcartwright commented Jan 22, 2020

This is one of those tricky edge cases where the badge service route needs to accept an optional param (the branch) that may or may not contain a /, and a required param (the workflow key) that may or may not contain a /.

I've not tried battling the route definition, but I don't see a pattern value we could use for the route without moving one of those two to a query param. Think I see a way we could make that switch to support this valid use case, with a redirector to maintain BC.

Hopefully I'm wrong though and there's an easy fix that can be made to the route def.

cc @badges/shields-maintainers

@bagbyte
Copy link
Author

bagbyte commented Jan 22, 2020

It seems this is due to the camp package which here is used as server, at this https://github.com/espadrine/sc/blob/2d0e9163d679d5ca60c9d597f0589a6c3e6c6a4a/lib/camp.js#L50 the uri is decoded, so when it hits your route, the information is already lost

@calebcartwright
Copy link
Member

calebcartwright commented Jan 22, 2020

@bagbyte - the issue/cause is understood, it's about resolution.

There's a few limitations in how the badge routes are matched, and this is one of those as detailed above.

The issue will need to be fixed by updating the route for the GH Workflow Status badge to account for the fact that the workflow names may include one or more / characters.

static get route() {
return {
base: 'github/workflow/status',
pattern: ':user/:repo/:workflow/:branch*',
}
}

@calebcartwright calebcartwright added bug Bugs in badges and the frontend service-badge Accepted and actionable changes, features, and bugs and removed question Support questions, usage questions, unconfirmed bugs, discussions, ideas labels Jan 22, 2020
@chris48s
Copy link
Member

I think @bagbyte has a point about the encoding.

You're right if we want to accept https://img.shields.io/github/workflow/status/bagbyte/actions/Deploy%20@bagbyte/lib (with a literal / character), but in principle we should be able to accept https://img.shields.io/github/workflow/status/bagbyte/actions/Deploy%20%40bagbyte%2Flib (with the / encoded as a %2F), shouldn't we?

@calebcartwright
Copy link
Member

calebcartwright commented Jan 26, 2020

I forget where, but feel like there's been some discussions in the past about how we currently cannot support badge route patterns that have consecutive route params which can both conditionally include a /.

/:foo/:bar/:bax+/:qux*

If that's something that can be fixed holistically upstream either in camp and/or path-to-regexp, or if there's an incantation for pattern that can be used that'd be great, I'm just not aware of anything today that would allow us to support this scenario via those consecutive route params (encoding or otherwise). The only approach I see that's readily available is to use a query param instead of a route param for one of those.

All of the below scenarios are entirely valid and we need to be able to support them, IMO both with or without the / character in the param value being explicitly encoded (especially since we don't require it to be encoded in other badge routes AFAIK)

user: badges
repo: shields
workflow: foo/bar
branch: bug/7

/github/workflow/status/badges/shields/foo/bar/bug/7
/github/workflow/status/badges/shields/foo%2Fbar/bug%2F7

user: badges
repo: shields
workflow: foo
branch: bug-7

/github/workflow/status/badges/shields/foo/bug-7

user: badges
repo: shields
workflow: foo/bar
branch: none

/github/workflow/status/badges/shields/foo/bar
/github/workflow/status/badges/shields/foo%2Fbar

user: badges
repo: shields
workflow: foo
branch: bug/7

/github/workflow/status/badges/shields/foo/bug/7
/github/workflow/status/badges/shields/foo/bug%2F7

@chris48s
Copy link
Member

Right, yeah. I see what you mean: everything just gets gobbled by branch*

I think you're right - the only way to allow both to contain a / will be to move branch to a query param.

@calebcartwright
Copy link
Member

The only other thing I could think of would be to include a static route part inbetween the two params, something like:
/github/workflow/status/:owner/:repo/:workflow+

and
/github/workflow/status/:owner/:repo/:workflow+/branch/:branch+

@RedSparr0w
Copy link
Member

Is there a reason why it doesn't work like this?

https://img.shields.io/github/workflow/status/bagbyte/actions/Deploy%20%40bagbyte%252Flib

I feel like it should work, but it doesn't. 🤔

It should be decoded to:
https://img.shields.io/github/workflow/status/bagbyte/actions/Deploy @bagbyte%2Flib which should make this the workflow name Deploy @bagbyte%2Flib which I guess doesn't get converted to Deploy @bagbyte/lib when it is sent to the api.

@RedSparr0w
Copy link
Member

The only other thing I could think of would be to include a static route part inbetween the two params, something like:
/github/workflow/status/:owner/:repo/:workflow+

and
/github/workflow/status/:owner/:repo/:workflow+/branch/:branch+

Would this break existing users badges though?

@calebcartwright
Copy link
Member

Would this break existing users badges though?

Yes, but if we were to make that change or move one of the route params to a query param, we'd add a redirector to map from the old route to the new to prevent any actual breakage.

@calebcartwright calebcartwright self-assigned this Mar 9, 2020
@paulmelnikow
Copy link
Member

Yes, but if we were to make that change or move one of the route params to a query param, we'd add a redirector to map from the old route to the new to prevent any actual breakage.

I’m not sure we could distinguish between an old-style request for myworkflow/mybranch and a new-style request for mytwopart/workflow.

If the ID of the workflow can’t be changed to accommodate the badge, an alternative could be a second route that pulls everything from the query params (like /endpoint) which can be used for these rare cases where the workflow contains a slash.

@calebcartwright
Copy link
Member

I’m not sure we could distinguish between an old-style request for myworkflow/mybranch and a new-style request for mytwopart/workflow.

Yup, was planning to also change part of the static portion of the route to account for this (probably something like /github/workflow/... --> /github/actions/workflow)

@paulmelnikow
Copy link
Member

Gotcha. Yea, true.

I think I prefer the query param version. It seems more readable to a human:

/github/actions/workflow/my%20complex/workflow/branch/main
/github/actions?workflow=my%20complex/workflow?branch=main

@calebcartwright
Copy link
Member

Yeah I do too. There's a handful of other cases where the branch had to be a query param for similar reasons. Only caveat on the leading portion of the route is that there are different constructs within GHA that could each have their own badges, for example a workflow status badge vs a job status badge, and potentially down the road additional CI related badges like test results (I've heard rumors Github are working on this)

@paulmelnikow
Copy link
Member

It's weird to me that we have github and workflow in the name, but not actions, which I think is the more identifiable and distinguishing name.

I wonder if we should use /github-actions (or even /actions).

Maybe /github-actions/status/:user/:repo/:workflow*?branch=branch? Nothing stops us from adding /github-actions/job-status, but the thing you're more likely to use – workflow status – would not be so wordy.

@calebcartwright
Copy link
Member

It's weird to me that we have github and workflow in the name, but not actions, which I think is the more identifiable and distinguishing name.

Blame GitHub marketing 😆 Actions is the buzzy word that everyone sees and uses, but regardless of the branding and marketing, an Action is a small reusable component that is incorporated within a job, and workflows consist of one or more jobs. I already see a wide array of configurations, with some having a single workflow with multiple jobs (1 job for linux build, 1 job for mac, etc.) and others having multiple, single job workflows.

The rough analogy would be if CircleCI had originally launched with Orb's and used Orb in all the branding and marketing.

For that matter, GH may very well make metadata/statistics available on published actions that action authors might be interested in having a badge for as well https://github.com/marketplace?type=actions

Not trying to speculate on potential future badges for the sake of speculation, more just trying to think through the route in the context of very real possible future asks to avoid locking us into a route that seems "wrong" down the road or end up with 5 different redirectors

@dbu
Copy link

dbu commented Dec 14, 2020

i just stumbled into this. is there any workaround? i am new to github workflows, but my understanding is they need to live in the .github/workflow/ directory, so we can't even have a workflow without slashes?

with travis-ci no longer being available freely for open source, i imagine a lot of projects moving to github actions around now and running into this issue...

@calebcartwright
Copy link
Member

@dbu - The name of a workflow and the directory path of the yaml file that contains the definition of a workflow are entirely separate things in this context.

i am new to github workflows, but my understanding is they need to live in the .github/workflow/ directory, so we can't even have a workflow without slashes?

We're obviously not GitHub so questions about the GitHub platform and it's features would be best addressed on their respective forums, but yes, GitHub requires the workflow yaml files to reside within a workflows subdirectory within the .github directory

https://docs.github.com/en/free-pro-team@latest/actions/learn-github-actions/introduction-to-github-actions#create-an-example-workflow

The workflow name is a metadata field that you as the user define within your respective yaml files, and this can largely contain whatever value you want (including apparently / characters)
https://docs.github.com/en/free-pro-team@latest/actions/reference/workflow-syntax-for-github-actions#name

As discussed above in this thread and shown on the Shields.io website for the GitHub Workflow Status badge, this workflow name is one of the badge route parameters:
https://img.shields.io/github/workflow/status/actions/toolkit/toolkit-unit-tests

with travis-ci no longer being available freely for open source, i imagine a lot of projects moving to github actions around now and running into this issue...

GitHub Actions has been widely used for quite some time now, and we get a lot of traffic on our Workflow Status Badges (~2,350 such unique badge requests in the last 15 minutes, and 206,000 over the last 24 hours), so I'd push back on the characterization of this as a widespread problem.

The issue at hand here is for cases where a user has decided to specify a workflow name that includes a / character, which in my experience hasn't been terribly common.

i just stumbled into this. is there any workaround?

The easiest workaround is simply: don't use a / in your workflow name. If you really need/want to have an explicit / in your workflow name then you can't use the Shields.io workflow status badge at the moment. You could try using the native Github badge option that allows the utilization of the path to the workflow file instead (https://docs.github.com/en/free-pro-team@latest/actions/managing-workflow-runs/adding-a-workflow-status-badge#using-a-workflow-file-path) albeit without the features and customization options that Shields provides.

At some point I'm sure we'll get around to extending the Shields badge offering to support this use case, but like all open source projects the maintainer team has limited bandwidth with a massive backlog, and this one hasn't really been a priority so far.

@dbu
Copy link

dbu commented Dec 16, 2020

@calebcartwright thank you very much for this expansive explanation even though you are not github support ❤️

i did not realize that setting a name in the workflow allows me to do what i wanted. i did not have one and assumed the default name from the path is the only name i can have.

thank you so much for taking the time to explain this to me. indeed, when one can set the name, then this is a very minor problem.

michalinacienciala added a commit to keep-network/keep-core that referenced this issue Jun 17, 2021
We no longer use CircleCI to test/build the `keep-core` code. We need to
replace the CircleCI build status badge in the projec'ts README with the
similiar badges for GitHub Actions builds.
Two badges have been introduced in place of the old one:
* Contracts (showing status of the last `Solidity` workflow executed on
`master` branch and triggered by the push event),
* Go client (showing status of the last `Go` workflow executed on
`master` branch and triggered by the push event).

The badges are displayed using `shields.io` tool, which was already used
for the Discord badge.

Also a badge was added in the README of the Token Dashboard:
* Token Dashboard / Testnet (showing status of the last `Token Dashboard
/ Testnet` workflow on `master` branch and triggered by the push event).

This time standard GitHub badge was used, as `shields.io` tool currently
does not support workflows with slash character in their name (see
badges/shields#4559).
michalinacienciala added a commit to keep-network/keep-core that referenced this issue Jun 18, 2021
We no longer use CircleCI to test/build the `keep-core` code. We need to
replace the CircleCI build status badge in the projec'ts README with the
similiar badges for GitHub Actions builds.
Two badges have been introduced in place of the old one:
* Contracts (showing status of the last `Solidity` workflow executed on
`master` branch and triggered by the push event),
* Go client (showing status of the last `Go` workflow executed on
`master` branch and triggered by the push event).

The badges are displayed using `shields.io` tool, which was already used
for the Discord badge.

Also a badge was added in the README of the Token Dashboard:
* Token Dashboard / Testnet (showing status of the last `Token Dashboard
/ Testnet` workflow on `master` branch and triggered by the push event).

This time standard GitHub badge was used, as `shields.io` tool currently
does not support workflows with slash character in their name (see
badges/shields#4559).
michalinacienciala added a commit to keep-network/keep-core that referenced this issue Jun 18, 2021
We no longer use CircleCI to test/build the `keep-core` code. We need to
replace the CircleCI build status badge in the projec'ts README with the
similiar badges for GitHub Actions builds.
Two badges have been introduced in place of the old one:
* Solidity build (showing status of the last `Solidity` workflow
executed on `master` branch and triggered by the push event),
* Go build (showing status of the last `Go` workflow executed on
`master` branch and triggered by the push event).

The badges are displayed using `shields.io` tool, which was already used
for the Discord badge.

Also a badge was added in the README of the Token Dashboard:
* Token Dashboard / Testnet (showing status of the last `Token Dashboard
/ Testnet` workflow on `master` branch and triggered by the push event).

This time standard GitHub badge was used, as `shields.io` tool currently
does not support workflows with slash character in their name (see
badges/shields#4559).
@Freed-Wu
Copy link

Freed-Wu commented Oct 23, 2022

a user has decided to specify a workflow name that includes a / character, which in my experience hasn't been terribly common.

According to https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#name:

If you omit name, GitHub sets it to the workflow file path relative to the root of the repository.

That means:

---
# no name
on: # yamllint disable-line rule:truthy
  ...
jobs:
  ...

will be treated as .github/workflows/XXX.yml, a name including /.

So, a name including / is a default workflow name, which should be think is
a terribly common case.

@chris48s
Copy link
Member

There is a PR open at #8475 to switch us away from using the workflow name due to #8146 which would also make this a non-issue

michalinacienciala added a commit to threshold-network/token-dashboard that referenced this issue Nov 7, 2022
We're adding the badges to the README file.
We've:
- added a chat badge to point to the Threshold Network Discord server
- added a docs badge linking to the Threshold Network documentation
- added a badge showing the status of the workflow building the dashboard. The
  displayed status is based on the status of the most recently executed `Token
  Dashboard / CI` workflow triggered by the push to `main`.
  Note that unike other badges this badge is created using native GH workflow
  status badge functionality - that's because `shields.io` doesn't support yet
  workflows using `/` character in their name (see
  badges/shields#4559).
@chris48s
Copy link
Member

See #8671 for the details of the fix for this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bugs in badges and the frontend service-badge Accepted and actionable changes, features, and bugs
Projects
None yet
Development

No branches or pull requests

7 participants