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

dashboard: Add view for PR runs #23

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

afinn12
Copy link
Contributor

@afinn12 afinn12 commented Nov 6, 2024

Needed

To Do

  • Fetch scripts (Nightly vs. PR) are separate, but the workflows are run by the same .yml file.
    • Since PR data is updated more frequently than Nightly data, to reduce stale data, the PR fetch script could be run more frequently.
    • However, separate workflows lead to race conditions (race condition comes from both forcing and update and both rolling back a prior commit). This is why we reverted to one workflow .yml.
    • Running the Nightly script every hour is redundant since the data updates nightly.
      • Possible solutions:
        • Add trigger to fetch data when PR is merged (with a delay so tests can complete)
        • Don't back out the data

Description

  • Fetch .js
    • Added access token to raise rate limits from 60 --> 5,000 per hour
    • Created script to fetch data on PRs
  • Fetch .yml
    • Run both fetch scripts at the start of every hour
  • index.js
    • Added buttons to switch view (Nightly vs PR)
    • Modified rowExpansionTemplate to show information based on view
      • Displays result information across all runs/PRs (see below)

Testing

Nightly View:
image

PR View
image

@afinn12 afinn12 changed the title Pr view dashboard: add PR view Nov 6, 2024
@afinn12 afinn12 changed the title dashboard: add PR view dashboard: Add view for PR runs Nov 6, 2024
@afinn12 afinn12 force-pushed the pr-view branch 3 times, most recently from cdc6a92 to 8405c7e Compare November 6, 2024 02:20
@afinn12 afinn12 mentioned this pull request Nov 6, 2024
1 task
Copy link
Contributor

@a1icja a1icja left a comment

Choose a reason for hiding this comment

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

Changes changes changes D:

.github/workflows/fectch-ci-data.yml Outdated Show resolved Hide resolved
scripts/fetch-ci-pr-data.js Outdated Show resolved Hide resolved
scripts/fetch-ci-pr-data.js Outdated Show resolved Hide resolved
pages/index.js Outdated Show resolved Hide resolved
pages/index.js Outdated Show resolved Hide resolved
pages/index.js Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@afinn12 afinn12 force-pushed the pr-view branch 2 times, most recently from df17116 to af17d13 Compare November 7, 2024 18:15
@afinn12 afinn12 marked this pull request as ready for review November 7, 2024 18:16
pages/index.js Outdated
Comment on lines 9 to 18
const [loading, setLoading] = useState(true);
const [jobs, setJobs] = useState([]);
const [checks, setChecks] = useState([]);
const [rowsPR, setRowsPR] = useState([]);
const [rowsNightly, setRowsNightly] = useState([]);
const [expandedRows, setExpandedRows] = useState([]);
const [display, setDisplay] = useState("nightly");
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we revert the spacing changes here? While it looks nicer, it can be annoying to maintain without requiring a linter.


// Set token used for making Authorized GitHub API calls.
// In dev, set by .env file; in prod, set by GitHub Secret.
require('dotenv').config();
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this work when ran by GH Actions?

}
const json = await response.json();
fetch_count++;
return await json;
Copy link
Contributor

Choose a reason for hiding this comment

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

return await is an anti-pattern. You can use just return.

@@ -146,78 +122,16 @@ function get_job_data(run) {
conclusion: null,
jobs: [],
};
if (run["status"] == "in_progress") {
if (run["status"] === "in_progress") {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason we're increasing how strict we are? Generally curious.



// Set token used for making Authorized GitHub API calls
require('dotenv').config();
Copy link
Contributor

Choose a reason for hiding this comment

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

Same question as above

Added a script that fetches PR data and created a separate view on the dashboard.
Tweaked dotenv require.

Fixes kata-containers#1

Signed-off-by: Anna Finn <[email protected]>
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