Open Med16-11 opened 1 year ago
@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.
Okay so a few things we can improve:
Okay so a few things we can improve:
- The key and value column should be a dynamic table where when you input data into the last row, it will automatically generate a new row.
- key and value should be an input form ( currently it is static)
- When hover on to a row, you should be able to delete it, similar to POSTMAN where a little trash icon will appear and you can just delete the row.
- The top box which displays the teutholgoy-suite command should automatically generate the command based on the kv that is given in table.
- run and dry-run button should have red and blue colors.
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:
import Button from "@mui/material/Button";
the color for run button can be used as color="error"
for now. For dry-run button try to find a blue color that matches the color="error"
.It looks like this now.
I realized that you have a merge commit, we can avoid that by doing:
Pending merge conflict to be resolved
Pending merge conflict to be resolved
On it.
That's how table looks now in dark mode.
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:
@Med16-11 I've noticed that:
- The input box for the key column cannot be edited?
- If we tick the checkbox for a bunch keys/values and delete it, the command box up top still retains the value of the deleted row.
Okay, I'll look into it.
@Med16-11 thanks for your new change, you're very close,
@kamoltat @zmc The schema for /suite endpoint on teuthology-api is defined and restrictive. Since the API does not allow the freedom to add any other args to the teuthology-suite command, the KEY column's can be plain text (listing all possible args) instead of input fields. This would let the user know about the restrictions of key names. Otherwise we would need to check the key names before requesting teuthology-api and throw an error if its not supported. What do you think?
@Med16-11 Good work! I know it's WIP, just letting you some of the issues:
@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 Select
and allow adding items to the table from that list.
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
Okay so I think I'm fine with having all the suite
keys as an out of the box experience. But from a Teuthology user perspective I only use around 10 - 13 out of 50+ arguments. If I were to use pulpito-ng to schedule jobs I would probably like to have only 10-13 keys in my key column with like at least 2-3 keys being selected by default. Therefore, I think we should keep the key delete functionality and key add button.
having all the keys displayed in the table kind of defeats the purpose of making the job scheduling experience more friendly
Name | Link |
---|---|
Latest commit | d876adc9582dd03bd083c09b63052cc2ae10dfd7 |
Latest deploy log | https://app.netlify.com/sites/pulpito/deploys/663e87baecd2d9000856460a |
Deploy Preview | https://deploy-preview-23--pulpito.netlify.app |
Preview on mobile | Toggle QR Code...Use your smartphone camera to open QR code link. |
To edit notification comments on pull requests, go to your Netlify site configuration.
@Med16-11 Thanks for making the changes, the feature is nearly complete
A few comments from looking at the changes:
teuthology-suite
as an option in the drop down box, it should be fixed on the command line box.I just re-pushed this branch after:
main
, solving all merge conflictspackage-lock.json
changes from two commits (dependencies were unchanged)/schedule
to Schedule
Made some changes:
NOTE:
Currently, there is a bug in removing the row, when deleting the row, the command box doesn't immediately remove the key and value associated with the row that has been removed. It's not a logic issue, since if we look at the localStorage in the browser, the rowData array updates correctly. I suspect that because of the async nature of JS, the problem is in:
const handleDeleteRow = (index) => {
console.log("handleDeleteRow");
const newRowData = [...rowData];
newRowData.splice(index, 1);
setRowData(newRowData);
const updatedRowCount = rowCount - 1;
setRowCount(updatedRowCount);
updateCommandBar();
};
where newRowData.splice(index, 1);
and setRowData(newRowData);
are still processing, while JS decides to run updateCommandBar();
first.
Made some changes:
- Rewrote the components with Material UI
- Improved OnListener logics
- Improved how we store data in useLocalStorage.
NOTE:
Currently, there is a bug in removing the row, when deleting the row, the command box doesn't immediately remove the key and value associated with the row that has been removed. It's not a logic issue, since if we look at the localStorage in the browser, the rowData array updates correctly. I suspect that because of the async nature of JS, the problem is in:
const handleDeleteRow = (index) => { console.log("handleDeleteRow"); const newRowData = [...rowData]; newRowData.splice(index, 1); setRowData(newRowData); const updatedRowCount = rowCount - 1; setRowCount(updatedRowCount); updateCommandBar(); };
where
newRowData.splice(index, 1);
andsetRowData(newRowData);
are still processing, while JS decides to runupdateCommandBar();
first.
This is Fixed!
NOTE: I always forget to drop the eb587902af6f6d3e96ef421ddf250a59a671a9e5 since the PR that has this commit hasn't been merged yet. So reviewer please help watch out for this hot reload commit.
Nevermind, the PR for that commit is merged so it is no longer cherry-picked to my dev local branch
In order, to test this PR properly, we need to modify the dry_run
args in teuthology_api
diff --git a/src/teuthology_api/services/suite.py b/src/teuthology_api/services/suite.py
index 8f1d0f7..af29aaa 100644
--- a/src/teuthology_api/services/suite.py
+++ b/src/teuthology_api/services/suite.py
@@ -22,8 +22,8 @@ def run(args, dry_run: bool, send_logs: bool, access_token: str):
)
try:
args["--timestamp"] = datetime.now().strftime("%Y-%m-%d_%H:%M:%S")
- if dry_run:
- args["--dry-run"] = True
+ if args["--dry-run"] == True:
+ log.info("This run is a dry run")
logs = logs_run(teuthology.suite.main, args)
return {"run": {}, "logs": logs}
(Edit: https://github.com/ceph/teuthology-api/pull/48 resolved this)
ran into this error:
Run npm run lint
> pulpito-ng@0.1.0 lint
> eslint src
/home/runner/work/pulpito-ng/pulpito-ng/src/lib/teuthologyAPI.ts
30:22 error React Hook "useUserData" is called in function "doSchedule" that is neither a React function component nor a custom React Hook function. React component names must start with an uppercase letter. React Hook names must start with the word "use" react-hooks/rules-of-hooks
Error: Process completed with exit code 1.
Therefore, I decided to change the name doSchedule
to useSchedule
Hey @kamoltat, I was testing this PR and I keep running into the following issue when I send a dry-run schedule request:
docker-compose-teuthology_api-1 | INFO: 192.168.96.1:47482 - "POST /suite?logs=true HTTP/1.1" 307 Temporary Redirect
docker-compose-teuthology_api-1 | INFO: 192.168.96.1:47468 - "OPTIONS /suite/?logs=true HTTP/1.1" 200 OK
docker-compose-teuthology_api-1 | 2024-06-10 11:08:12,547.547 INFO:root:teuthology version: 1.1.1.dev689+g3f548c60
docker-compose-teuthology_api-1 | 2024-06-10 11:08:12,548.548 INFO:teuthology.suite:Using random seed=3261
docker-compose-teuthology_api-1 | 2024-06-10 11:08:12,548.548 INFO:teuthology.suite.run:kernel sha1: distro
docker-compose-teuthology_api-1 | Process Process-1:1:
docker-compose-teuthology_api-1 | Traceback (most recent call last):
docker-compose-teuthology_api-1 | File "/usr/local/lib/python3.8/dist-packages/teuthology/suite/run.py", line 200, in choose_ceph_hash
docker-compose-teuthology_api-1 | ceph_hash = util.git_ls_remote(
docker-compose-teuthology_api-1 | File "/usr/local/lib/python3.8/dist-packages/teuthology/suite/util.py", line 163, in git_ls_remote
docker-compose-teuthology_api-1 | return repo_utils.ls_remote(url, branch)
docker-compose-teuthology_api-1 | File "/usr/local/lib/python3.8/dist-packages/teuthology/repo_utils.py", line 76, in ls_remote
docker-compose-teuthology_api-1 | raise GitError(e.output) from None
docker-compose-teuthology_api-1 | teuthology.exceptions.GitError: b"fatal: could not read Username for 'https://github.com': No such device or address\n"
docker-compose-teuthology_api-1 |
docker-compose-teuthology_api-1 | During handling of the above exception, another exception occurred:
docker-compose-teuthology_api-1 |
docker-compose-teuthology_api-1 | Traceback (most recent call last):
docker-compose-teuthology_api-1 | File "/usr/lib/python3.8/multiprocessing/process.py", line 315, in _bootstrap
docker-compose-teuthology_api-1 | self.run()
docker-compose-teuthology_api-1 | File "/usr/lib/python3.8/multiprocessing/process.py", line 108, in run
docker-compose-teuthology_api-1 | self._target(*self._args, **self._kwargs)
docker-compose-teuthology_api-1 | File "/teuthology_api/src/teuthology_api/services/helpers.py", line 62, in _execute_with_logs
docker-compose-teuthology_api-1 | job_count = func(args)
docker-compose-teuthology_api-1 | File "/usr/local/lib/python3.8/dist-packages/teuthology/suite/__init__.py", line 153, in main
docker-compose-teuthology_api-1 | run = Run(conf)
docker-compose-teuthology_api-1 | File "/usr/local/lib/python3.8/dist-packages/teuthology/suite/run.py", line 57, in __init__
docker-compose-teuthology_api-1 | self.base_config = self.create_initial_config()
docker-compose-teuthology_api-1 | File "/usr/local/lib/python3.8/dist-packages/teuthology/suite/run.py", line 92, in create_initial_config
docker-compose-teuthology_api-1 | ceph_hash = self.choose_ceph_hash()
docker-compose-teuthology_api-1 | File "/usr/local/lib/python3.8/dist-packages/teuthology/suite/run.py", line 203, in choose_ceph_hash
docker-compose-teuthology_api-1 | raise util.schedule_fail(message=str(e), name=self.name, dry_run=self.args.dry_run) from None
docker-compose-teuthology_api-1 | File "/usr/local/lib/python3.8/dist-packages/teuthology/suite/util.py", line 77, in schedule_fail
docker-compose-teuthology_api-1 | raise ScheduleFailError(message, name)
docker-compose-teuthology_api-1 | teuthology.exceptions.ScheduleFailError: Scheduling VallariAg-2024-06-10_11:08:11-teuthology:no-ceph-main-distro-default-smithi failed: b"fatal: could not read Username for 'https://github.com': No such device or address\n"
docker-compose-teuthology_api-1 | 2024-06-10 11:08:14,164.164 ERROR:teuthology_api.services.helpers:Scheduling VallariAg-2024-06-10_11:08:11-teuthology:no-ceph-main-distro-default-smithi failed: b"fatal: could not read Username for 'https://github.com': No such device or address\n"
docker-compose-teuthology_api-1 | INFO: 192.168.96.1:47482 - "POST /suite/?logs=true HTTP/1.1" 400 Bad Request
Please let me know if you see the problem on your end too.
@VallariAg Yes I was able to recreate the issue, fixing ...
@VallariAg ceph-repo
should be https://github.com/ceph/ceph-ci.git
@kamoltat ohhhh my bad! I'll test this again today.
Really exhaustive error handling, this is awesome! I was able to schedule runs from the /schedule page and tested features with different teuthology-suite config, works great for me!
https://github.com/ceph/pulpito-ng/assets/22287000/6f899554-5323-4390-8533-f5189fe489bc
This PR is dependent on the following PRs:
https://github.com/ceph/teuthology-api/pull/51 https://github.com/ceph/teuthology/pull/1924
Testing this PR
please checkout -b the PRs above for teuthology and teuthology-api.
And in teuthology-api do