Skip to content

Modify commitee TOML and workflow#67

Closed
jaisnan wants to merge 19 commits into
model-checking:mainfrom
jaisnan:modify-2
Closed

Modify commitee TOML and workflow#67
jaisnan wants to merge 19 commits into
model-checking:mainfrom
jaisnan:modify-2

Conversation

@jaisnan

@jaisnan jaisnan commented Aug 22, 2024

Copy link
Copy Markdown

This adds @carolynzech to the committee file and modifies the workflow to prevent existing checks from being re-used. This will create a more reliable status check.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 and MIT licenses.

@jaisnan jaisnan requested a review from a team as a code owner August 22, 2024 15:54
});
}

// Simplify to not use github create at all

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.

Why was this seemingly necessary before, but turns it it actually isn't?

@celinval celinval left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

BTW, will this work if the approval was dismissed?

Comment on lines +9 to +10
pull_request:
branches: [ main ]

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Why do you need this?

# To protect against that scenario, we can turn on number of approvals required to 2 in the github settings
# of the repository
on:
workflow_dispatch:

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This doesn't run in the context of a PR though, does it? You could add an option to provide a PR link or a PR number. But I don't know if it's worth it

# for trying to modify the pull request for newer project.
# Source: https://stackoverflow.com/a/76994510
permissions: write-all
push:

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

why push?

const tomlContent = fs.readFileSync('.github/pull_requests.toml', 'utf8');
console.log('TOML content:', tomlContent);
const tomlData = toml.parse(tomlContent);
console.log('Parsed TOML data:', JSON.stringify(tomlData, null, 2));

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Have you considered the business logic to a script instead of hard coding it in here? I think it is usually much easier to debug and run it locally. For example, you could create a script that takes as an argument a list of logins and the script succeeds if and only if the minimum number of people in the committee has been achieved.

Or the script output the number of people from the committee from that list and you only check here if the number is >= minimum requirement.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Thank you for that idea! I shall move the logic to its own script ad use gh command line to try and rerun old failing checks.

@jaisnan

jaisnan commented Aug 27, 2024

Copy link
Copy Markdown
Author

Closing this approach in favor of #74 which is simpler and more reliable. Starting afresh to test the necessary conditions as well.

@jaisnan jaisnan closed this Aug 27, 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.

5 participants