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

Make merge gatekeeper more resilient #67

Closed
wants to merge 2 commits into from

Conversation

franklad
Copy link

Addressing Issue: #64 by adding a round tripper to perform retries.

* ground works

* cleanup

* comments
Copy link
Contributor

@rytswd rytswd left a comment

Choose a reason for hiding this comment

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

I left a few comments about the markdown related CI failure 🙏

Also, the new logic looks OK, but it would be great if you could add some test cases to ensure we got the right implementation in place ☺️

README.md Outdated
@@ -16,7 +16,7 @@ At UPSIDER, we have a few internal repositories set up with a monorepo structure

We are looking to add a few more features, such as extra signoff from non-coder, label based check, etc.

<sup><sub>NOTE:
Copy link
Contributor

Choose a reason for hiding this comment

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

If you take out the blank spaces here, it would change the line break layout slightly. While it seems to read just fine with this change, I'd like to check if this was an intentional update?

Copy link
Author

Choose a reason for hiding this comment

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

my ide removed it

Copy link
Contributor

Choose a reason for hiding this comment

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

This is something very peculiar to our setup, but we are using Importer https://github.com/upsidr/importer for our documentation. This means we are only pulling in pieces of documentation from docs directory.

You can see some markdown comments such as

<!-- == imptr: inputs / begin from: ./docs/action-usage.md#[inputs] == -->

It means it's checking the file ./docs/action-usage.md and a marker inputs, and when you run importer update README.md, it would replace the documentation with that given content. This is our own way of ensuring documentation does not get drifted between places, and that's the reason why the CI is failing. If you could make sure to update the ./docs directory content as well as README.md, that would be appreciated! 🥰

Copy link
Author

Choose a reason for hiding this comment

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

addressed

@rytswd
Copy link
Contributor

rytswd commented Jul 8, 2023

Btw, I'm so sorry for the extremely delayed review 🙇
We will try to get our review cycles in place for future changes -- please do not hesitate to mention myself if you are not getting any response 🙏

* ground works

* cleanup

* comments

* use importer to update docs

* tests

* rerender
@franklad
Copy link
Author

Hi @rytswd, thanks for your comments and sorry for my delayed response too!
I added some tests and updated the docs. would appreciate another look! cheers

@franklad franklad closed this Mar 10, 2024
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.

2 participants