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

Write setup script mac #822

Closed
wants to merge 8 commits into from
Closed

Write setup script mac #822

wants to merge 8 commits into from

Conversation

TheseusGrey
Copy link
Contributor

@TheseusGrey TheseusGrey commented Sep 14, 2018

Created the basic structure of the setup script and implemented the setup for mac. To use the script simply run python aimmo-setup.py from the aimmo root, it will then go through the process of setting up aimmo, then all you need to do is get the unity package, and open up docker to finalise it's setup. The script itself will inform the user when/how it fails if it does. A common issue i found when running this was running setting up the fronend using yarn, it didn't always run the command properly, i will however look into making this section more reliable.


This change is Reviewable

Paris Goldman-Smith added 7 commits September 14, 2018 12:17
structure of the setup for mac has been implemented, now it just needs debugging and testing to make sure it works.
fixed some issues surround certain command within the mac setup not working. These should have been fixed now.
added comments and documentation for the script and it's functions, along with some warnings about use.
added a new way of doing the frontend dependencies part of the setup that should solve the unreliability issue.
@TheseusGrey TheseusGrey self-assigned this Sep 17, 2018
Copy link
Contributor

@mrniket mrniket left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 files reviewed, 6 unresolved discussions (waiting on @dent50cent)

a discussion (no related file):
Does the script print the output of the commands to the console? If not is it possible to have a "verbose" option to do so?



aimmo-setup.py, line 45 at r1 (raw file):

# First we find and store the OS we are currently on, 0 if we didn't figure it out
# Although if you're not using one the options above for development what are you doing with your life.

we should remove this comment


aimmo-setup.py, line 64 at r1 (raw file):

print('---------------------------------------------------------------------------------------------------')
print('| Welcome to aimmo! This script should make your life alil easier, just be kind if it doesnt work |')

Maybe we could add, please ask another member of the team if the script doesn't work?

Also, a bit of explanation of what the script does could be useful


aimmo-setup.py, line 65 at r1 (raw file):

print('---------------------------------------------------------------------------------------------------')
print('| Welcome to aimmo! This script should make your life alil easier, just be kind if it doesnt work |')
print('| You may be asked to enter your password during this setup                                       |')

is it possible to get a confirmation from the user if they want to continue?


aimmo-setup.py, line 72 at r1 (raw file):

    This executes the sequence of shell commands needed in order to set up aimmo. At present if changes are made
    to the setup process or versions (such as minikube version) changes, then it will need to be updated manually.
    In future it would be nice to have it automatically find the version we need at the time.

Usually, we put feature requests like these inside of GitHub issues, they tend to get lost here


aimmo-setup.py, line 74 at r1 (raw file):

    In future it would be nice to have it automatically find the version we need at the time.

    It would also be nice to implement the ability to automate getting the unity package, however this will require

Usually, we put feature requests like these inside of GitHub issues, they tend to get lost here

Copy link
Contributor Author

@TheseusGrey TheseusGrey left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 files reviewed, 6 unresolved discussions (waiting on @dent50cent)

a discussion (no related file):

Previously, mrniket (Niket Shah) wrote…

Does the script print the output of the commands to the console? If not is it possible to have a "verbose" option to do so?

Currently no, it should be fairly simple to implement. However this may not be particularly informative in cases like the pipenv install command, as it's output is a progress bar, and i'm not such how this would interact with the way python calls it.



aimmo-setup.py, line 45 at r1 (raw file):

Previously, mrniket (Niket Shah) wrote…

we should remove this comment

this was removed while implementing the linux setup


aimmo-setup.py, line 64 at r1 (raw file):

Previously, mrniket (Niket Shah) wrote…

Maybe we could add, please ask another member of the team if the script doesn't work?

Also, a bit of explanation of what the script does could be useful

i added "this script should get the various requirements for AI:MMO and set them up for you.", is this okay or should i say something different?


aimmo-setup.py, line 65 at r1 (raw file):

Previously, mrniket (Niket Shah) wrote…

is it possible to get a confirmation from the user if they want to continue?

yes, this has now been implemented


aimmo-setup.py, line 72 at r1 (raw file):

Previously, mrniket (Niket Shah) wrote…

Usually, we put feature requests like these inside of GitHub issues, they tend to get lost here

This was more for my reference while making the script so I didn't forget. I can remove it from here and create a GitHub issue for it so it doesn't get lost.


aimmo-setup.py, line 74 at r1 (raw file):

This was more for my reference while making the script so I didn't forget. I can remove it from here and create a GitHub issue for it so it doesn't get lost.

changes based off of review, and adjusted code to reflect the refactor done in the linux setup issue
Copy link
Contributor

@mrniket mrniket left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r2.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @mrniket and @dent50cent)


aimmo-setup.py, line 54 at r2 (raw file):

def install_yarn(operatingSystem):
    '''
    :param operatingSystem: values from the OStypes dict. (Should be updated to enum once python 3 is availible)

we should add the stuff in the parenthesis as an issue


aimmo-setup.py, line 56 at r2 (raw file):

    :param operatingSystem: values from the OStypes dict. (Should be updated to enum once python 3 is availible)

    OS dependant, so it must be passed to this function in order to run correctly.

are these the right docstring descriptions? I see this:

✂-1

Copy link
Contributor

@mrniket mrniket left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @mrniket and @dent50cent)

a discussion (no related file):

Previously, dent50cent wrote…

Currently no, it should be fairly simple to implement. However this may not be particularly informative in cases like the pipenv install command, as it's output is a progress bar, and i'm not such how this would interact with the way python calls it.

I think it would be useful to have the option in the case that things go wrong


Copy link
Contributor

@mrniket mrniket left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @mrniket and @dent50cent)


aimmo-setup.py, line 64 at r1 (raw file):

Previously, dent50cent wrote…

i added "this script should get the various requirements for AI:MMO and set them up for you.", is this okay or should i say something different?

ah I see, yeah should be fine

Copy link
Contributor

@OlafSzmidt OlafSzmidt left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @dent50cent and @OlafSzmidt)

a discussion (no related file):

Previously, dent50cent wrote…

Currently no, it should be fairly simple to implement. However this may not be particularly informative in cases like the pipenv install command, as it's output is a progress bar, and i'm not such how this would interact with the way python calls it.

I think it would be quite useful to have a verbose option too. You can pipe commands to a logging module quite easily.



aimmo-setup.py, line 147 at r2 (raw file):

# If kubernetes version changes this will need updating

Not necessarily. You can do this automatically too if you wish:

Use PyGithub (already introduced in the unity project, loads of use cases in this file for it too)
Access minikube repo by ID
Get a GitRelease object for the latest release of minikube
GetGitReleaseAsset's, use labels to distinguish between linux mac or windows (each release contains all 3 in their assets, differentiated by labels)
Download that asset and run the same commands as before.


aimmo-setup.py, line 175 at r2 (raw file):

inderpendant

typo


aimmo-setup.py, line 182 at r2 (raw file):

./game_frontend

I don't like this because this method is called in both Mac & Linux setups which aren't the same. Prone to failure if the order of execution of commands will change in the future to add something etc.

A decent solution I see:

  • Use absolute paths (for example tell the user to run this script from the root of aimmo, as soon as they do that call os.getcwd() and work based on that).

aimmo-setup.py, line 234 at r2 (raw file):

print('AI:MMO already present in /etc/hosts...')

Shouldn't be a print, we should use a logging library and set this as a warning or something.

Meaning all other prints should be a logging.info or something call instead imo


aimmo-setup.py, line 267 at r2 (raw file):

it's

its


aimmo-setup.py, line 309 at r2 (raw file):

it's

its


aimmo-setup.py, line 310 at r2 (raw file):

in aimmo/aimmo/static/unity  (folder may not exist yet)

Can you not do this automatically?

Pull from github releases API,
make the folder if it doesn't exist,
move the file in,
unpack it,
remove the zip

Copy link
Contributor

@OlafSzmidt OlafSzmidt left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @mrniket and @dent50cent)


aimmo-setup.py, line 54 at r2 (raw file):

Previously, mrniket (Niket Shah) wrote…

we should add the stuff in the parenthesis as an issue

I think it's fine because a lot of things will be changed when we port the repo to python3, if we document them all we will pollute the issues

Typo in "availible", also watch your line length:

https://github.com/ocadotechnology/aimmo/blob/master/CONTRIBUTING.md

@faucomte97 faucomte97 deleted the write_setup_script_mac branch September 12, 2019 16:54
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.

None yet

4 participants