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.
Please check for things like commented code before committing :)
There was a problem hiding this comment.
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:
|
|
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 |
|
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) |
screen-capture.mp4 |
|
ran into this error: Therefore, I decided to change the name |
|
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.
We can probably reuse the new CodeBlock component now because that PR got merged.
There was a problem hiding this comment.
Yep Code block seems like a good idea
| "--newest", | ||
| "--num", | ||
| "--limit", | ||
| "--priority", |
There was a problem hiding this comment.
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.
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! |
Signed-off-by: Med16-11 <singhmedhavi923@gmail.com> resolved conflits
Signed-off-by: Med16-11 <singhmedhavi923@gmail.com>
Signed-off-by: Zack Cerza <zack@redhat.com>
1. Rewrote the components with Material UI 2. Improved OnListener logics 3. Improved how we store data in useLocalStorage. Signed-off-by: Kamoltat Sirivadhna <ksirivad@redhat.com>
…ling suites Use axios post request to for run, dry-run and force-priority Signed-off-by: Kamoltat Sirivadhna <ksirivad@redhat.com>
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 <ksirivad@redhat.com>
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 <ksirivad@redhat.com>




screen-capture.mp4
Merge this first ceph/teuthology#1924, followed by ceph/teuthology-api#51 before merging this PR!
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