-
Notifications
You must be signed in to change notification settings - Fork 154
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
Change approach for Prism fixtures #2081
Comments
Speaking of annoyances, this is totally unrelated but I wanted to bring this up for a while now and this seems like as good a place as any: The thing that annoys me the most about contributing here is the label check (which doesn't even compare against the occasional git status notice). Every PRs workflow here fails which sends me a notification to both the GitHub app and I get an email as well. I understand what you're going for with that check, but would it makes sense to open an issue thinking about alternate ways to achieve the same thing? |
There were a few times only fixtures caught issues about my change, so making it CI-only may make debugging harder.
Yeah that makes complete sense, as you probably can't address that yourself as a contributor. |
I agree with Stan, I would not make the fixture stuff CI only. Just this morning while working on #2098 the fixtures caught a bug in my implementation. Regarding the label stuff, even if we applied the I don't know how we can address this to be honest. We can to block merging so that maintainers are reminded to label the PRs before shipping, but don't want the failure to show up for contributors. |
Regarding the labels: We had an idea to only run that check once the PR is approved by a maintainer, who could be responsible for adding the label. But unfortunately there is no option for that to be a trigger: https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#pull_request |
There seems to be an event for when a pull request review is submitted. We'd probably need to hook into that one. |
Or how about this: #2122 |
We've changed the labelling behaviour so that it only runs that check after approval: #2123 We've leave the Prism fixtures approach alone for now. |
In #1090, we added Prism as a submodule so that we could test against the latest fixtures.
This was useful previously, but now that Prism is very stable, we rarely see any failures.
The current approach has an annoyance for developers, because it means
git status
will often show there are changed files, e.g.:modified: test/fixtures/prism (new commits)
I'd like to propose that we remove the submodule reference, but continue to test against the fixtures in CI only, using a slightly modified approach, e.g. cloning the Prism repo as a CI step.
The text was updated successfully, but these errors were encountered: