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.
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
feat: create task #207
base: main
Are you sure you want to change the base?
feat: create task #207
Changes from 2 commits
06602d1
b5a917d
3fa2eca
ffecf22
0acd4b0
dd14159
2948508
7f21d1f
71d9be1
1a56253
41d8d19
0dfe256
e4993df
2ac6959
a4edd0e
8586b4d
0d7981a
3eaad7f
c45724b
b182a87
b4570a5
925d63a
11976e1
c6e3964
5607e62
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 find this much harder to read/parse than:
Why the function and the use of
not
?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.
Also, if your client wrapper already checked for
CONFLICT/409
type errors and returned these asKubernetesConflict
exceptions, you could design this more elegantly by letting thewhile
loop conditional take care of managing the retries and then handle the "too many retries" situation below the loop with an explicitInternalServerError
message. Then you could simplify this check to:All the other errors you can just let bubble up. I'm pretty sure FOCA will handle these gracefully (as mentioned below).
I think this would be considerably cleaner - and more informative.
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.
Ahh did you mean something like 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.
Is it a base class for a TESK request (any request, including
GET
requests) or a task request (aPOST
requests totasks/
in order to request a task being created). If the former, make sure to rename the module and class, if the latter, make sure to adapt the module-level docstring.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 the former, can you please suggest a name, I can't think of a reason or name to do so. I name it task request so that this base class can hold all the attr that might be common among endpoint that deal with "task request" and create a common interface to interacting with api programatically.
If we extend more endpoint sooner or later (say create serviceInfo), then I would propose to even create a base class for this class named
request
or something, just so that programatically, business logic is forced to exit via the same method sayresponse
.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.
Yeah, that second part is where I would see this going then. Maybe even ending up in FOCA.
I mean, if we already have 21 changed files (not including tests) for the addition of a single controller, we might as well go all the way, right? 😛
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.
@JaeAeich: Why is it that with your PRs I often see the entire file changed, even though it's just an iteration over the last time I've viewed? I don't have this with other people, so I guess it's something with your editor or Git flow.
Please have a look at this, because it's really quite annoying. Instead of just focusing on what changed since the last time, I have to look through the entire file again - which is not only not fun, but also holds up the reviews big time, especially when they tend to end up being huge.
And even apart from that, it's also really not good practice in terms of provenance. If this were previously existing code (e.g., maybe some of the old TES code from TES-core still remains), you'd end up being listed as
git blame
for every line, taking credit and blame for other people's work.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.
Yeah I agree, but I can assure you am not going it on purpose 😭, the thing is TESK code is complex and if I try to break the code base it makes no sense and is very hard to connect the dots. Also I don't think I have any issues in my git flow, my configs seem to be sound. I am trying not to step and cover up someones code but if you notice we most of the files are completely new and not a modification of prev (well there weren't prev files to speack of as I am mostly working on new module
api
mostly andservice
isn't touched).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.
What's the significance of this? Why not going straight for the Pydantic way of marshalling the model?
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 tried, for some reason I couldn't, IDK why! And this weird way only worked.