-
Notifications
You must be signed in to change notification settings - Fork 8
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
Added job scheduling page #23
base: main
Are you sure you want to change the base?
Conversation
@Med16-11 always rebase against latest main branch before filing a PR, or else there will likely be merge conflicts like what you see right now. |
src/pages/Schedule/index.js
Outdated
</button> | ||
</div> | ||
</div> | ||
{/* <div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please check for things like commented code before committing :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I actually thought that we can use the commented code later to ensure that buttons are working properly and can write the logic in functions later.
Okay so a few things we can improve:
|
Got it. |
It seems like I have closed the PR accidentally. Could you please open it @kamoltat |
Thanks you for filling the change, here are some of the things I think can be improved:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Med16-11
Thanks for the change, I think with the table color being black it is hard to see in dark mode, might be worth also changing the border to white or light grey when in dark mode.
I realized that you have a merge commit, we can avoid that by doing:
Also, to keep the commits clean in your PR, you should try to squash your commits in to 1 commit whenever the change between the two commits serve the same purpose. You can do this by: commit your new change: In the interactive shell put an
Then it will redirect you to commit message , just edit the commit by deleting the random commit message you wrote and keep/edit the original commit message that you a squashing to. |
Pending merge conflict to be resolved |
On it. |
I'll be squashing all the commits after receiving your reviews. |
@Med16-11 Thanks for the table change the border looks good now for dark mode. |
@Med16-11 I've noticed that:
|
Okay, I'll look into it. |
@Med16-11 thanks for your new change,
|
@kamoltat @zmc @Med16-11
|
@VallariAg If I understand you correctly, you're suggesting we not allow/require the user to input the flag names in the "Key" column, and instead list all the available options and allow the user to fill in the ones they need. I think I agree with that. We could populate a This reminds me: at some point we'll want to be populating default values; perhaps we should add an endpoint to teuthology-api that provides these. |
@zmc that sounds perfect. I'll open an issue for adding an endpoint for default values on teuthology-api and using it on pulpito-ng. In teuthology meeting, we also discussed to disable editing on the command input above the table. We can focus on scheduling runs with the form table for now. I'll open a new issue for later enabling that feature. |
@zmc @VallariAg |
3957229
to
1b77720
Compare
NOTE: I always forget to drop the eb58790 Nevermind, the PR for that commit is merged so it is no longer cherry-picked to my dev local branch |
5cefc09
to
aff5afd
Compare
In order, to test this PR properly, we need to modify the
(Edit: ceph/teuthology-api#48 resolved this) |
Signed-off-by: Med16-11 <[email protected]> resolved conflits
Signed-off-by: Med16-11 <[email protected]>
Signed-off-by: Zack Cerza <[email protected]>
1. Rewrote the components with Material UI 2. Improved OnListener logics 3. Improved how we store data in useLocalStorage. Signed-off-by: Kamoltat Sirivadhna <[email protected]>
…ling suites Use axios post request to for run, dry-run and force-priority Signed-off-by: Kamoltat Sirivadhna <[email protected]>
screen-capture.mp4 |
ran into this error:
Therefore, I decided to change the name |
schedule feature uses useMutation to deal with cases like onSuccess, onError and isLoading. CircularProgress is added when button is clicked and mutation is in the state of isLoading. Signed-off-by: Kamoltat Sirivadhna <[email protected]>
1. After a run is scheduled, user will be able to see the results including the logs from teuthology-suite. Only success runs will show all logs from teuthology-suite, failed runs will only return the exception with descriptive failure reasons. 2. Added Warning Alerts for 0 jobs scheduled in a run 3. Added more arguments that can be use to schedule run Signed-off-by: Kamoltat Sirivadhna <[email protected]>
Hey @kamoltat,
Please let me know if you see the problem on your end too. |
@VallariAg Yes I was able to recreate the issue, fixing ... |
@VallariAg |
@kamoltat ohhhh my bad! I'll test this again today. |
{clickDryRun.isLoading | clickRun.isLoading | clickForcePriority.isLoading ? <CircularProgress /> : <Editor | ||
value={logText} | ||
readOnly={true} | ||
highlight={(logText) => highlight(logText, languages.yaml)} | ||
style={{ | ||
fontFamily: [ | ||
"ui-monospace", | ||
"SFMono-Regular", | ||
'"SF Mono"', | ||
"Menlo", | ||
"Consolas", | ||
"Liberation Mono", | ||
'"Lucida Console"', | ||
"Courier", | ||
"monospace", | ||
].join(","), | ||
textAlign: "initial", | ||
}} | ||
/>} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can probably reuse the new CodeBlock
component now because that PR got merged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep Code block seems like a good idea
"--newest", | ||
"--num", | ||
"--limit", | ||
"--priority", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I schedule runs, they are all end with "dead" status because for docker setup the "<config_yaml>": ["/teuthology/containerized_node.yaml"]
config is needed. I hard-coded that in request and it resulted with "pass" status!
Right now, t-api input of <config_yaml>
is a path (like the above example) but there's a PR open which will take in actual config text: ceph/teuthology-api#50. Nevertheless, I guess the react coded would probably be the same for both.
So we probably need to add support for config_yaml
to get green runs in docker. It's a list
type though which sounds complex so maybe we can implement this in next PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I never got to that part where I was looking for a passed run, so thank you for reaching that.
Really exhaustive error handling, this is awesome! |
screen-capture.mp4
This PR is dependent on the following PRs:
ceph/teuthology-api#51
ceph/teuthology#1924
Testing this PR
please checkout -b the PRs above for teuthology and teuthology-api.
And in teuthology-api do