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

Reorder book summary #236

Merged
merged 2 commits into from
Nov 15, 2021
Merged

Reorder book summary #236

merged 2 commits into from
Nov 15, 2021

Conversation

erlend-sh
Copy link
Member

No description provided.

@erlend-sh erlend-sh requested review from tigleym and cdsupina November 9, 2021 13:15
Copy link
Collaborator

@tigleym tigleym left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks!

@erlend-sh
Copy link
Member Author

erlend-sh commented Nov 9, 2021

@orhun could this summary file be excerpted from future formatting & linting checks? It's not like like the other .md files.

Also we seem to still be running build checks on .md files?
Screenshot 2021-11-09 at 15 59 02

@orhun
Copy link
Member

orhun commented Nov 9, 2021

@orhun could this summary file be excerpted from future formatting & linting checks? It's not like like the other .md files.

Right now, we're supposedly ignoring *.md files, do you want this file to be included in build checks or something else? I didn't quite get it.

Also we seem to still be running build checks on .md files?

It is weird. The "checks" tab shows 0 so I guess it is being excluded from builds?

checks

Unlike #231:

checks

Also, I'm not sure what "Waiting for status to be reported" means, but I found this.

Either way, build checks for this PR seems to be not run so I assume this is an issue with GitHub. Can somebody with write access check if this button is shown:

approve-and-run

It is highly unlikely but maybe clicking this will clear the "Expected — Waiting for status to be reported" message. Not really sure.


One last thing: I replicated this PR on my fork and build checks didn't run: orhun#2

@erlend-sh erlend-sh closed this Nov 11, 2021
@erlend-sh erlend-sh reopened this Nov 11, 2021
@tigleym
Copy link
Collaborator

tigleym commented Nov 11, 2021

I suspect this action is causing the checks to not go through: "Create and Publish Github Release". Interestingly, it also happens on my own fork of the repo (and orhun's it seems).

In particular: this error Error: ⚠️ GitHub Releases requires a tag. I don't totally understand why a local PR would trigger this action vs. one from a forked repo (at least this is what I've observed from the PRs with successful checks here).

Some digging around I found this: softprops/action-gh-release#20

@orhun
Copy link
Member

orhun commented Nov 11, 2021

I suspect this action is causing the checks to not go through: "Create and Publish Github Release". Interestingly, it also happens on my own fork of the repo (and orhun's it seems).

In particular: this error Error: ⚠️ GitHub Releases requires a tag. I don't totally understand why a local PR would trigger this action vs. one from a forked repo (at least this is what I've observed from the PRs with successful checks here).

Some digging around I found this: softprops/action-gh-release#20

Yes, I noticed this as well. Pretty weird behavior. I don't know why it runs "on its own" when there is not tag created either.

Maybe we should consider switching to a more stable action for uploading release assets, such as upload-release-action.

(Although I don't know if it is related to the issue we're having here.)

@tigleym
Copy link
Collaborator

tigleym commented Nov 11, 2021

Yes, I noticed this as well. Pretty weird behavior. I don't know why it runs "on its own" when there is not tag created either.

I read that create will also trigger workflows for newly created branches: https://docs.github.com/en/actions/learn-github-actions/events-that-trigger-workflows#create. I'm not sure if not specifying a branch here is causing the workflow to run when any new branch is created.

Maybe we could do something like:

on:
  create:
    branches:
      - 'release*'
    tags:
      - 'v*'

EDIT: Okay the above approach didn't seem to do anything.

Another thing we could try is instead of running on create we could do it on push?

Maybe we should consider switching to a more stable action for uploading release assets, such as upload-release-action.

Definitely worth looking into!

@orhun
Copy link
Member

orhun commented Nov 12, 2021

I read that create will also trigger workflows for newly created branches: docs.github.com/en/actions/learn-github-actions/events-that-trigger-workflows#create. I'm not sure if not specifying a branch here is causing the workflow to run when any new branch is created.

Exactly! Good catch 🐟

Another thing we could try is instead of running on create we could do it on push?

That's definitely going to solve the "running on its own" issue I think. I always used push for CD workflows in my projects and every time it worked fine. I totally overlooked that release workflow was using create in this case 😦

Maybe we should consider switching to a more stable action for uploading release assets, such as upload-release-action.

Definitely worth looking into!

Can I make this changes and submit a PR if you're not already working on this? 🐻

@tigleym
Copy link
Collaborator

tigleym commented Nov 12, 2021

Can I make this changes and submit a PR if you're not already working on this? 🐻

I haven't worked on it. So it would be great if you could do that! 🙏

@erlend-sh
Copy link
Member Author

@orhun could we temporarily disable whatever check is blocking this just to get it merged?

@orhun
Copy link
Member

orhun commented Nov 15, 2021

@orhun could we temporarily disable whatever check is blocking this just to get it merged?

Checks in the actual repository are not being run for pull requests. Only the workflows in your forked repository (at the time of PR commit - 0d4a515) are checked. So you can disable the workflows by updating your erlend-sh-patch-1 branch. But, I don't think that's the reason why we can't merge because there seems to be a merge conflict.

I can solve the merge conflict and try to merge the PR for you, if you want 🐻

@erlend-sh
Copy link
Member Author

erlend-sh commented Nov 15, 2021

I can solve the merge conflict and try to merge the PR for you, if you want 🐻

Yes please! 🙏

@orhun
Copy link
Member

orhun commented Nov 15, 2021

I resolved the conflict, but still couldn't merge because of this message:

Screenshot 2021-11-15 at 22-24-05

Then I decided to go into repository settings and saw that we have the main branch protected and it was requiring some status checks to be passed before merge:

Screenshot 2021-11-15 at 22-23-24 Branch protection rule

But the problem is, we don't run these checks for *.md files. So we're stuck in a dilemma 🙂

Should I remove those settings or temporarily disable the exclusion of markdown files? (I'd go with 1️⃣st option since this might cause other problems in the future) What do you think? @erlend-sh

@erlend-sh
Copy link
Member Author

1️⃣ 👍

@orhun orhun merged commit dacb638 into main Nov 15, 2021
@orhun orhun deleted the erlend-sh-patch-1 branch November 15, 2021 21:18
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