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

ci: New workflow to update issues #3889

Merged
merged 9 commits into from Jan 25, 2022
Merged

ci: New workflow to update issues #3889

merged 9 commits into from Jan 25, 2022

Conversation

joeyparrish
Copy link
Member

This replaces an internal tool that we have been running on a private
Jenkins server.

This replaces an internal tool that we have been running on a private
Jenkins server.
@joeyparrish joeyparrish added the type: CI An issue with our continuous integration tests label Jan 20, 2022
Copy link
Contributor

@TheModMaker TheModMaker left a comment

Choose a reason for hiding this comment

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

Since the internal version had tests, I wonder if we should just copy the previous tests over to reference and convert later? I would suggest adding a bug to track adding tests for this.

const core = require('@actions/core');

const octokit = github.getOctokit(process.env.GITHUB_TOKEN);
const [owner, repo] = process.env.GITHUB_REPOSITORY.split('/');
Copy link
Contributor

Choose a reason for hiding this comment

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

These should probably be all-caps since they're constants even if they're dynamic.

Copy link
Member Author

Choose a reason for hiding this comment

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

I prefer to keep them as-is, since it means I can use this syntax later in the queries:

{
  owner,
  repo,
}

Instead of:

{
  owner: OWNER,
  repo: REPO,
}

.github/workflows/tools/update-issues/issues.js Outdated Show resolved Hide resolved
.github/workflows/tools/update-issues/issues.js Outdated Show resolved Hide resolved
.github/workflows/tools/update-issues/issues.js Outdated Show resolved Hide resolved
@joeyparrish
Copy link
Member Author

Since the internal version had tests, I wonder if we should just copy the previous tests over to reference and convert later? I would suggest adding a bug to track adding tests for this.

Done.

TheModMaker
TheModMaker previously approved these changes Jan 21, 2022
const ARCHIVE_AFTER_CLOSED_DAYS = 60;


async function archiveOldIssues(issue) {
Copy link
Collaborator

@theodab theodab Jan 22, 2022

Choose a reason for hiding this comment

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

This is an operation on a single issue, even though the name specifies "Issues". You should probably rename it to archiveOldIssue. Same idea with the rest of these functions.

Copy link
Member Author

Choose a reason for hiding this comment

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

I disagree. They accept a single issue at a time, so that the loop over issues is somewhat more efficient, but they are inherently filtering through issues (plural). If you try to describe what a function does in terms of a single issue, you end with much wordier names like "archiveIssueIfOld".

}

async function manageWaitingIssues(issue) {
// Find all waiting issues.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This comment does not seem representative, since this method is not responsible for traversing the array of issues. Only manage waiting issues in this step. perhaps?

Copy link
Member Author

Choose a reason for hiding this comment

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

Reworded.

async function maintainMilestones(issue, nextMilestone, backlog) {
// Set or remove milestones based on type labels.
if (!issue.closed) {
if (issue.hasAnyLabel(LABELS_FOR_NEXT_MILESTONE)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps it would make sense to add priority: P4 issues to the backlog, even if they are type: docs? We have a few p4 docs issues right now, and they are all marked as backlog.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done. I've set P3 and P4 as defaulting to the backlog in spite of the type.

const ALL_ISSUE_TASKS = [
archiveOldIssues,
unarchiveIssues,
reopenIssues,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since the reopenIssues step happens after the archiveOldIssues step, does that mean that we might run into a situation where someone asks to re-open an issue at the very last moment and it archives instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. The original tool may have has this problem as well, since it was based on a python decorator and no explicit ordering. Fixed by moving reopenIssues to the top.

@joeyparrish joeyparrish merged commit a65a1cd into shaka-project:master Jan 25, 2022
joeyparrish added a commit that referenced this pull request Jan 25, 2022
This ports over internal tests suggested in #3889.
@github-actions github-actions bot added the status: archived Archived and locked; will not be updated label Jul 25, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
status: archived Archived and locked; will not be updated type: CI An issue with our continuous integration tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants