-
Notifications
You must be signed in to change notification settings - Fork 1
Kate/resources #2
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
Conversation
Thank you for the comments @doublebyte1 @PascalLike ! This is very helpful.
|
Maybe for now, you could just switch off the validation for providers, when loading the file? I think the users will be confused by this. |
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.
Thank you for the changes, @KatKatKateryna ! 👍🏽 I think you addressed most of the issues. We only need a few more tweaks.
- The refactoring of the provider class looks 💯
- The provider edition looks much more friendly now. 🥇
- The initial validation dialog is also easier to read, but I still think this error about providers is confusing (I left a comment about it).
- The approach for crs validation seems reasonable; the only issue is when there is no Internet access.
- I tried to activate the GitHub action with no success. Maybe you could tell me if Iḿ doing something wrong? (:
Please check these repo settings (can only be accessed by admin) and if adding permissions doesn't help, I'll make a setup for a custom repo-scoped token 🙏 |
@KatKatKateryna Thank You! Doing it at organisational level did the trick 👍🏽 |
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.
Thank you for doing the changes so quickly! 🚀
It all looks good to me now.
We'll figure out how to do a provider validation later.
models/top_level/utils.py
Outdated
return False | ||
return True | ||
|
||
else: # lighter request (if content is not needed) |
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.
It is fine to keep it; I just wanted to understand why (👍🏽
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.
LGTM! Thanks @KatKatKateryna!
Something to consider for future work #3 |
Edits of the existing code
New changes
Limitations:
Screenshots