Merging the account manager into the plugin #134
Conversation
There was a problem hiding this comment.
I would like to see this series of commits cleaned up:
- Each commit should have a clear purpose and should be clearly described. There should be a short descriptive subject followed by information about why the changes are necessary.
- There should not be multiple commits with the same subject.
- Please check messages for spelling and grammar.
- Typo fixes and other cleanups should be folded into other commits.
|
I will likely need to close this PR and open a new one, hopefully with a lot more cleanup and up to standard |
Please don't do that. Just update this PR. If you need help with that, let me know. |
|
Great work on cleaning up the commits. One final advice. You can see that when you merged the main branch back into this working branch, it introduced a series of extraneous commits into your history. It's generally better to rebase your working branch into the main branch. Please look into and remove the merge commits from your history. I suggest you backup your work too, just in case something goes wrong modifying the history. See this Stackoverflow answer quickly describing the differences between the two workflows https://stackoverflow.com/questions/804115/when-do-you-use-git-rebase-instead-of-git-merge |
QuanMPhm
force-pushed
the
iss_114
branch
3 times, most recently
from
69b3b6a
to
e06e459
Compare
|
@knikolla I've resolved all comments so far. The things I see that are left to do is to rename the OS_AUTH_URL attribute, and update the test files as you see fit. I will wait for further instructions. |
| raise NotFound(f"{response.status_code}: {response.text}") | ||
| elif 'already exists' in response.text: | ||
| raise Conflict(f"{response.status_code}: {response.text}") | ||
| openshift_token = os.getenv(f"OPENSHIFT_{var_name}_TOKEN") |
There was a problem hiding this comment.
Instead of passing in a token, can we just read the configuration from a kubeconfig file? Using config.load_kubeconfig() will work with both the in-cluster configuration for a serviceaccount and with a local kubeconfig file (and respects the KUBECONFIG environment variable).
If you don't want to change this now, it may be worth turning into an issue.
There was a problem hiding this comment.
@knikolla What is your opinion on this?
There was a problem hiding this comment.
For now we can keep passing the token as an environment variable, as that preserves the original behavior. We can work out a better way in the future.
There was a problem hiding this comment.
@QuanMPhm please create an issue to resolve this later, link it here and then resolve this conversation.
| self.logger.info(f"msg: user created ({unique_id})") | ||
| return | ||
|
|
||
| raise Conflict("400: " + f"user already exists ({unique_id})") |
There was a problem hiding this comment.
Avoid including http response codes in our text messages. We're already providing the response code to the client as part of the HTTP response. Also, we should be returning a 409 status code on conflicts.
There was a problem hiding this comment.
@larsks @knikolla Currently I am handling the exceptions the same way that the current code is handling it, by passing the response and the message. Should I still remove all error codes?
As for whether the Conflict exception returns the appropriate code, that doesn't seem to be implemented yet. Conflict is simply subclassed from the built-in Exception class. Let me know if I should implement it.
There was a problem hiding this comment.
From what I've read about Django exception handling, there is no way to raise custom exceptions in Django which returns custom status codes in the http response, especially not in a signal handler, as is the case with openshift.py.
When an error is raised (as in the example you highlighted), Django by default returns 500 as the status code.
The only way I can see to return appropriate errors is by including it in the error message. Please tell me if I missed something and if there is a clean way of returning custom errors. Otherwise, I've edited the error messages to contain the appropriate status code.
There was a problem hiding this comment.
We don't need to return custom error codes in Django. These exceptions are purely for the code itself to handle, not any external user.
There was a problem hiding this comment.
@QuanMPhm if you have made this change please resolve this conversation or ask one of those in the conversation to resolve it if you are not sure.
| self.logger.info(f"msg: user created ({unique_id})") | ||
| return | ||
|
|
||
| raise Conflict("409: " + f"user already exists ({unique_id})") |
There was a problem hiding this comment.
Please remove this line as it will always be called regardless. We don't need to raise an exception here in case of conflict.
The purpose of the exception block below is to make sure that all Conflict exceptions thrown by the Kubernetes client are caught so that execution of the code can continue.
Please add some functional tests to see what happens in cases of attempting to create an already existing user and make sure the code continues to work.
There was a problem hiding this comment.
@QuanMPhm if you removed the line please resolve this conversation.
| return self.check_response(r) | ||
| if self.client.project_exists(project_id): | ||
| self.logger.info(f"msg: project exists ({project_id})") | ||
| return |
There was a problem hiding this comment.
You're not returning anything here?
There was a problem hiding this comment.
It seems the purpose of _get_role and _get_project is purely to check if the role or project has been created on the Openshift cluster. I've actually broke the test cases here by converting the exception raise to a log message.
There was a problem hiding this comment.
@knikolla what are your thoughts on this approach?
QuanMPhm
force-pushed
the
iss_114
branch
3 times, most recently
from
c446f15
to
e76b430
Compare
|
Blocked on Functional test. |
…the plugin to communicate with any Openshift cluster
|
@knikolla I have brought over the unit tests from the account manager and hopefully resolved all problems with the functional tests |
|
@QuanMPhm what are the next steps? |
Closes #137, I am merging the account manager into the plugin. Most changes are in
openshift.py