Disabled github auth for dev setup and set up default as development #66 #69
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@VallariAg can you kindly check the task and merge the pull request. |
VallariAg
reviewed
Sep 10, 2024
There was a problem hiding this comment.
Hello @AyishikD, thanks for opening this PR. I've left a few comments.
Additionally, please merge the three commits into one. Also here are the instructions to sign commits: https://docs.github.com/en/authentication/managing-commit-signature-verification/signing-commits
AyishikD
force-pushed
the
main
branch
2 times, most recently
from
558bf92 to
00c2c25
Compare
|
@VallariAg kindly check now and regarding the random line changes I pasted the original code from github to remove them but they are still there. Rest changes I have done. |
|
@VallariAg can you kindly recheck and notify me if any changes are needed else merge the changes. Thank you in advance. |
|
@VallariAg I completely deleted my commit and added only the required lines still random new lines with @@ are coming . I tried git diff too the more I change the more it increases or remains. I think the new line additions in the middle may have caused it since line numbers are increased. Sorry to say I cannot fix the random new lines cause the more I try more complex they become. |
|
@VallariAg I checked in diffchecker too even the software told me no extra blank lines or less blank lines are present in between the two files except my changed lines. |
VallariAg
reviewed
Sep 12, 2024
There was a problem hiding this comment.
@AyishikD I think you've done a great job removing the random new lines - I only see a couple of them now!
You can try out VS Code: https://code.visualstudio.com/docs/sourcecontrol/overview#_viewing-diffs
It has amazing view of looking at diffs which would help you to only keep the intentional-made changes before making a commit.
I see that your commit message now includes title of all the commits you merged (bunch of repeated lines of sign-offs and "removed github...").
You can change the commit message to be more neat (example: 3798208).
To edit your commit message, you can look up about git commit --amend command.
4 tasks
AyishikD
force-pushed
the
main
branch
2 times, most recently
from
d471604 to
60f2368
Compare
|
@VallariAg I have added the extra line gaps through vs code source control where they were previously. I don't think any line blanks are less or more now. Also, I have amended the commit message as you gave in the example. Thank you. |
|
Hii @VallariAg did u check the latest commit? |
|
Hi @VallariAg its been 2 days since I pushed the last commit. Please can you kindly check it? |
VallariAg
reviewed
Sep 16, 2024
There was a problem hiding this comment.
Nice work, it's looking more neater now. I've left a couple more comments. The commit seems to have 1 extra commit title and sign-off, we can probably remove that too.
Also we can revisit the commit description. Example, "- suite.py route: Removed GitHub auth requirement during development." - this is no longer true because we removed this check from routes/suite.py from last iteration of review.
More importantly...
I haven't tested the PR locally yet (and I'll probably only have time to do it later in week). You can test it by sending a request to /suite endpoint to ensure it works after this change. This can be done by using FastAPI Swagger UI (found at /docs endpoint on a browser) and sample payload in README. Please understand that reviews might sometimes take a couple days, especially if there's a weekend. Thanks!
Signed-off-by: AyishikD <[email protected]> Disabled GitHub authentication for the development setup. This change affects the following files: - `main.py`: Modified to skip the `access_token` check during development. - `services`: Updated logic to bypass auth checks when in development mode.
|
@VallariAg I did the changes u mentioned. Tried testing locally where it gave error "ERROR: Could not find a version that satisfies the requirement ansible-core==2.16.6 (from teuthology)" Some kind of json error I guess |
|
@AyishikD I recommend you to try the docker setup to test /suite (it's the easier method) - the instructions are in README. About the invalid JSON, if you're using the sample payload from README, make sure you have removed the comment starting with |
While setting up in docker
|
|
@AyishikD I have never seen that error before, can you share the steps you took to reach here? |
Yeah suree |
|
@AyishikD It would be more helpful to share commands you ran and their outputs - it's hard to reproduce a problem with vague instructions. But looking at the error you shared before, I think the problem could be that you're supposed to run "./start.sh" in See if you're following these steps: https://github.com/ceph/teuthology-api/blob/main/gh-actions/start.sh |
Yeah as per instruction I executed the start.sh file
|
In the teuthology repo I had only changed the docker.compose file after which I had again resetted it but still the same error came. |
|
@VallariAg any update mam? |
|
@AyishikD I still don't understand the cause of the problem. If you're available, you can join "Teuthology weekly" call today at 8pm IST using this link https://meet.jit.si/ceph-teuthology. |
Sure mam I am joining today only in meet. |
|
@VallariAg fixed postgresql error by using the commands and in the 01-init.sh changed to But now error is coming as unhealthy :( |
|
@VallariAg At ubuntu distro in wsl same error on running ./start.sh Bootstrap error.. |
|
Hii @VallariAg sorry for disturbing , did u check it? |
|
Sorry I was on vacation, happy diwali @AyishikD :) |
@VallariAg my Ubuntu version is 22.04.05 LTS. Also, the code that sir told me in the meeting is running and creating a container, but the code which is actually here in teuthology does not work and gives an error. Thank you mam for your so much guidance. |
|
@VallariAg Finally I have successfully run it. I would be grateful if u kindly allow me to write a starting document on it so that in the future someone using Windows and WSL can easily do it. The container is created and everything is running but the final output is coming as I dont know why paddle is unhealthy but container is created. |
|
Well @VallariAg made paddles healthy but strangely only 4/11 containers are running |
|
@AyishikD I tried to run the docker setup on Github Actions's windows machines but I didn't have much success either. That probably could be why failure in 1 container cascades into failures in many other containers. I'm not sure why teuthology and testnodes services are failing. Can you rerun and share what you see when running
that's a great idea, we probably need to document the process for windows users once we work it all out. |
|
@VallariAg after running ./start.sh in WSL this is showing And in normal git bash this is showing |
|
@VallariAg I tried with gunicorn in WSL too and the results are: On running : The output coming is: And in the terminal where I started the server its showing Thank You |
|
And @VallariAg in the docker the error I studied like it could not connect to postgreSQL This is the main error: |
|
This looks like a network problem to me. Looking at |
|
Hey @AyishikD, I got an opportunity to access a windows machines, so I tried to setup teuthology on it and saw the exact problems you were seeing. I was finally able to get it working by doing this: For setup, I used git-bash and Docker Desktop (apparently it uses WSL under the hood!). I didn't see the network problems you saw, but let's see the output of the above commands. |
Wait mam I am following the steps and showing you I saw my mistake I didnt convert testnode using dos2unix |
|
@VallariAg The complete results, well same error I tried it in git bash. |
Looks like you might have to run dos2unix for /teuthology_api/start_container.sh too. |
|
@VallariAg network error I guess but why?:( |
|
Can you see if If ping is not successful, we can try solutions from here maybe: https://forums.docker.com/t/dns-broken-on-docker-desktop-for-windows/130121 |
|
@VallariAg here is the complete log: |
|
@AyishikD sorry for late replies! Looks like ping seems to work. You can try to run Please share output of: Looking at your logs from #69 (comment), I expect at least this to be successful. |
|
@VallariAg here is the complete log |
|
@VallariAg only 3 containers are running |
|
@AyishikD you can look at output of This seems odd to me because paddles was healthy in this run here: #69 (comment). |
|
@VallariAg this is the output And sorry for late reply had an on campus interview. |
|
|
@VallariAg Mam did you see it? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Disabled github auth for dev setup and set up default as development
Fixes #66
Changed and removed github auth for development in suite.py route, main.py and the services part.
The access_token check is skipped during dev setup.
By default deployment is set to development if not explicitly mentioned
Checklist