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: small enhancements #20

Merged
merged 1 commit into from
Nov 22, 2024

Conversation

afinn12
Copy link
Contributor

@afinn12 afinn12 commented Nov 5, 2024

Important

  • Review basePath changes in next.config.js. In local production deployment, changes were needed to work.
    • (changes to basePath also made in deploy.yml)
  • Review Automatic Filter Note at bottom

Description

  • Changed basePath in next.config.js/deploy.yml

  • Added win-dev for Windows development

  • Upgrade Next.js version/include dotenv for dev

  • Simplified weatherTemplate.js

    • weatherIcon is calculated by fails/runs instead of (fails+skips)/runs
    • Removed getWeatherIcon
    • Removed console.assert()
    • Error check that idx is not > icons.length
  • index.js

    • Moved total rows to the top of table
    • Updated dev data to ../localData/job_stats.json (instead of ../job_stats.json)
    • Filters automatically by most fails
  • Added files in /localData to .gitignore

  • fetch-ci-nighty-data.js

    • added token to increase rate limits
    • removed extra code for PR data
    • reformat fetches --> show the status number and message
  • Updated readme with script/token instructions

Testing

  • Tested in dev

    • TODO: BUG: In windows, running:
      node --require dotenv/config .\scripts\fetch-ci-nightly-data.js > localData\job_stats.json

    • Will produce the error:
      image

    • This is caused by "> localData\job_stats.json" and has only been produced in windows dev

    • Instead of > localData\job_stats.json, doing CTRL + C and CTRL + V from tmp-data.json to job_stats.json is a temporary solution that avoids these unexpected tokens.

  • Tested in prod
    in prod (using build/start):
    image

Automatic Filter Note

As seen above, the weather doesn't correlate with the number of fails (if a test was run more/less)

Sorting by weather produces:
image

  • This is less ideal because tests with only 1 run/fail are prioritized
    • We are working on a solution to combine folder names with their contents, as these appear to be the "jobs" with one run.
    • Q: Should we filter by fails or weather as the default?

@afinn12 afinn12 marked this pull request as ready for review November 5, 2024 21:32
@afinn12 afinn12 changed the title Small enhancements dashboard: small enhancements Nov 6, 2024
@afinn12 afinn12 force-pushed the small-enhancements branch from a80d308 to b396b29 Compare November 6, 2024 02:30
@afinn12 afinn12 mentioned this pull request Nov 6, 2024
6 tasks
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.

Definitely some changes that need to be made, but they're all minor

Copy link
Contributor

Choose a reason for hiding this comment

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

Some context on the changes in this file:

This allows for us to have a staging instance by hosting in another repo without constantly modifying the code

components/weatherTemplate.js Outdated Show resolved Hide resolved
components/weatherTemplate.js Outdated Show resolved Hide resolved
next.config.js Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
pages/index.js Outdated Show resolved Hide resolved
@afinn12 afinn12 force-pushed the small-enhancements branch from 87e3b59 to 38c026b Compare November 6, 2024 17:46
@afinn12 afinn12 marked this pull request as draft November 6, 2024 18:22
@afinn12 afinn12 force-pushed the small-enhancements branch 2 times, most recently from f9ad24c to a82fb67 Compare November 7, 2024 00:35
@a1icja a1icja force-pushed the small-enhancements branch from a82fb67 to 839eefc Compare November 7, 2024 01:18
@afinn12 afinn12 force-pushed the small-enhancements branch 2 times, most recently from 4f9b1e5 to fb38fc0 Compare November 7, 2024 16:49
@afinn12 afinn12 marked this pull request as ready for review November 7, 2024 17:04
@afinn12 afinn12 mentioned this pull request Nov 7, 2024
3 tasks
Copy link
Collaborator

@sprt sprt left a comment

Choose a reason for hiding this comment

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

Thanks @afinn12! I left a few comments/questions.

For this PR, can you edit your commit message and list the same bullet points you listed in the PR description?

In future PRs (not this one), each such bullet point should be an individual commit (roughly, use your best judgement to split up into logical changes) to facilitate review. Potentially also split up into different PRs, as you'll notice that the review-correction cycle can delay merging: if you have multiple PRs, you can at least prioritize the features/PRs you want to get merged.

Crafting PRs is tricky! More info on this process here: https://github.com/kata-containers/community/blob/main/CONTRIBUTING.md#best-practices-for-patches
And some examples:
https://github.com/kata-containers/kata-containers/pull/10466/commits
https://github.com/kata-containers/kata-containers/pull/10038/commits

.github/workflows/deploy.yml Outdated Show resolved Hide resolved
.github/workflows/deploy.yml Show resolved Hide resolved
scripts/fetch-ci-nightly-data.js Outdated Show resolved Hide resolved
README.md Outdated
Create the .env file:
```bash
NODE_ENV=development
TOKEN=token <GITHUB_PAT_OR_OTHER_VALID_TOKEN>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you document or link to instructions on how to obtain this token?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will update.

Copy link
Collaborator

@sprt sprt Nov 11, 2024

Choose a reason for hiding this comment

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

Do you know roughly how close we are to the new limit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Current fetch count is 22 when I ran the script (very below limit, will increase with reruns)

.gitignore Outdated Show resolved Hide resolved
pages/index.js Outdated Show resolved Hide resolved
components/weatherTemplate.js Outdated Show resolved Hide resolved
scripts/fetch-ci-nightly-data.js Outdated Show resolved Hide resolved
scripts/fetch-ci-nightly-data.js Outdated Show resolved Hide resolved
pages/index.js Show resolved Hide resolved

// 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
Collaborator

Choose a reason for hiding this comment

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

Need to guard this for GHA

Updated fetch script to require access token, simplified/removed extra PR code, and added error message.
Modified basePath, styling change to weatherTemplate/table, added default sort by most Fails.
Added win-dev/localData, updated README/.gitignore.

Fixes: kata-containers#25

Signed-off-by: Anna Finn <[email protected]>
Create the folder /localData. Then, run:

```bash
node scripts/fetch-ci-nightly-data.js > localData/job_stats.json
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'll do this in another PR.

Suggested change
node scripts/fetch-ci-nightly-data.js > localData/job_stats.json
node --require dotenv/config scripts/fetch-ci-nightly-data.js > localData/job_stats.json

Comment on lines +82 to +88
- name: Build with Next.js (No base path)
if: ${{ env.NEXT_PUBLIC_BASE_PATH == '' }}
run: ${{ steps.detect-package-manager.outputs.runner }} next build

- name: Build with Next.js (With base path)
if: ${{ env.NEXT_PUBLIC_BASE_PATH != '' }}
run: NEXT_PUBLIC_BASE_PATH=${{ env.NEXT_PUBLIC_BASE_PATH }} ${{ steps.detect-package-manager.outputs.runner }} next build
Copy link
Collaborator

Choose a reason for hiding this comment

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

We don't need a condition here. I'll fix this in another PR.

@sprt sprt merged commit 2b7e4c2 into kata-containers:main Nov 22, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants