Skip to content
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

Merge Feature/kubernetes 1 onto Develop #32

Open
wants to merge 10 commits into
base: develop
Choose a base branch
from

Conversation

KubaKrzych
Copy link
Contributor

Feel free to drop any advice on code or anything that should have been done differentely :)

Copy link
Contributor

@HermanPlay HermanPlay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requested changes:

  1. Remove pls empty file "_ _ init _ _.py"

General advise to use miniconda instead of anaconda in Dockerfiles (only for this taks ofc), since it will speed up deployment to docker (because of lower size of miniconda).

You actually implemented "singleton" pattern for one model, so to be consistent, you can do it for other models too.

Beside these remarks, very good solution, at least it works ;)

@KubaKrzych
Copy link
Contributor Author

Requested changes:

  1. Remove pls empty file "_ _ init _ _.py"

General advise to use miniconda instead of anaconda in Dockerfiles (only for this taks ofc), since it will speed up deployment to docker (because of lower size of miniconda).

You actually implemented "singleton" pattern for one model, so to be consistent, you can do it for other models too.

Beside these remarks, very good solution, at least it works ;)

The apis with init.py are not the ones we have made for the sake of this task. I had created this branch from develop branch and it has pulled the files from the first task.

Maybe you could drop me an info if I should somehow delete pulled changes (or revert them (?))from develop, since these include apis from the first task, and if so how should I do it?. @HermanPlay Also, if you want to checkout the changes that are important in this PR, you can check out the files in this path: tasks/02_kubernetes_fundamentals/kubernetes_1

If I could ask you to help me out with it? :)

@HermanPlay
Copy link
Contributor

HermanPlay commented Dec 6, 2022

Requested changes:

  1. Remove pls empty file "_ _ init _ _.py"

General advise to use miniconda instead of anaconda in Dockerfiles (only for this taks ofc), since it will speed up deployment to docker (because of lower size of miniconda).
You actually implemented "singleton" pattern for one model, so to be consistent, you can do it for other models too.
Beside these remarks, very good solution, at least it works ;)

The apis with init.py are not the ones we have made for the sake of this task. I had created this branch from develop branch and it has pulled the files from the first task.

Maybe you could drop me an info if I should somehow delete pulled changes (or revert them (?))from develop, since these include apis from the first task, and if so how should I do it?. @HermanPlay Also, if you want to checkout the changes that are important in this PR, you can check out the files in this path: tasks/02_kubernetes_fundamentals/kubernetes_1

If I could ask you to help me out with it? :)

Okay i understand, there is no need to change apis from the first task, i don't think you can somehow revert those changes, unless they were done as separate commit. You can ignore those changes, so they are not included during merging your branch into develop, read this for more info. click. I think research in this topic can help you.

Your solution for the second task is good, I approve it.

P.S. Tell me if you have any questions. If not, i will change my review to "Approve".

@KubaKrzych
Copy link
Contributor Author

Requested changes:

  1. Remove pls empty file "_ _ init _ _.py"

General advise to use miniconda instead of anaconda in Dockerfiles (only for this taks ofc), since it will speed up deployment to docker (because of lower size of miniconda).
You actually implemented "singleton" pattern for one model, so to be consistent, you can do it for other models too.
Beside these remarks, very good solution, at least it works ;)

The apis with init.py are not the ones we have made for the sake of this task. I had created this branch from develop branch and it has pulled the files from the first task.
Maybe you could drop me an info if I should somehow delete pulled changes (or revert them (?))from develop, since these include apis from the first task, and if so how should I do it?. @HermanPlay Also, if you want to checkout the changes that are important in this PR, you can check out the files in this path: tasks/02_kubernetes_fundamentals/kubernetes_1
If I could ask you to help me out with it? :)

Okay i understand, there is no need to change apis from the first task, i don't think you can somehow revert those changes, unless they were done as separate commit. You can ignore those changes, so they are not included during merging your branch into develop, read this for more info. click. I think research in this topic can help you.

Your solution for the second task is good, I approve it.

Thanks a lot, I'll check it out for sure. :)

@matined, before approving this PR, please give me time so I can fix this issue (or at least try to do so)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants