Skip to content

Include Reg into microk8s - #453

Closed
etheleon wants to merge 8 commits into
canonical:masterfrom
etheleon:feature/include-regDocker__registry-v2-CLI-tool
Closed

Include Reg into microk8s#453
etheleon wants to merge 8 commits into
canonical:masterfrom
etheleon:feature/include-regDocker__registry-v2-CLI-tool

Conversation

@etheleon

@etheleon etheleon commented May 4, 2019

Copy link
Copy Markdown

As suggested in this thread, reg will be a good addition to enable users to easily interact with local registry

@etheleon

etheleon commented May 4, 2019

Copy link
Copy Markdown
Author

@ktsakalozos , the tests are failing any idea why?

source $SNAP/actions/common/utils.sh

read -ra ARGUMENTS <<< "$1"
argz=("${ARGUMENTS[@]/#/--}")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I do not think you are doing anything with argz so lets simplify the code by removing these two lines.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Alright!


source $SNAP/actions/common/utils.sh

echo "Disabling Reg"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think a single message should be enough. :)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Alright!

REG_VERSION="${REG_VERSION:-v0.16.0}"
echo "Fetching reg version $REG_VERSION."
sudo mkdir -p "$SNAP_DATA/bin"
sudo "${SNAP}/usr/bin/curl" -L https://github.com/genuinetools/reg/releases/download/${REG_VERSION}/reg-linux-amd64 -o "$SNAP_DATA/bin/reg"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

MicroK8s ships for amd64 adn arm64, so we should use the $SNAP_ARCH to get the right binary from the releases. Something like this should work: https://github.com/genuinetools/reg/releases/download/${REG_VERSION}/reg-linux-${SNAP_ARCH}.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Alright!

@ktsakalozos

Copy link
Copy Markdown
Contributor

@etheleon thank you for this PR. I left a few comments. It would be great if you could somehow add a test for this addon. Here is how to do that:

If you follow the instruction and build MicroK8s from source https://github.com/ubuntu/microk8s#building-from-source you should see that you do not have a microk8s.reg command in your path. The reason is you are missing an entry into snapcraft.yaml like this one: https://github.com/ubuntu/microk8s/blob/master/snapcraft.yaml#L66 and the corresponding wrapper script like this one: https://github.com/ubuntu/microk8s/blob/master/microk8s-resources/wrappers/microk8s-linkerd.wrapper

It would have been better if the fist time you try the microk8s.req command the wrapper would download the binary then. This way you would not need the microk8s.enable reg step. What do you think?

@etheleon

etheleon commented May 6, 2019

Copy link
Copy Markdown
Author

@ktsakalozos Thanks for the detailed write up on writing the test! Let me get started on it! :)

- snapcraft: add reg as app
- validator: include validator for `reg`
  * tags busybox
  * push to local registry
  * assets presence of image
  * deletes tag
- microk8s.reg wrapper: installs reg if binary not found
- microk8s.enable reg: installs reg (might be duplicate)
@etheleon
etheleon force-pushed the feature/include-regDocker__registry-v2-CLI-tool branch from 71ebed9 to 44a3218 Compare May 20, 2019 06:13
@etheleon

etheleon commented May 20, 2019

Copy link
Copy Markdown
Author

Hi @ktsakalozos, made the following changes:

  • snapcraft: added reg as app

  • validator: include validator for reg

    • tags busybox
    • push to local registry
    • assets presence of image
    • deletes tag
  • microk8s.reg wrapper: installs reg if binary not found. Based on your suggestion here. Yes i agree!

It would have been better if the fist time you try the microk8s.req command the wrapper would download the binary then. This way you would not need the microk8s.enable reg step. What do you think?

  • microk8s.enable reg: installs reg (might be duplicate)

Should we keep this?

@ktsakalozos

Copy link
Copy Markdown
Contributor

@etheleon are you able to see this: https://travis-ci.org/ubuntu/microk8s/builds/534665856?utm_source=github_status&utm_medium=notification ?

I believe you are missing an import.

       validate_reg()
      NameError: global name 'validate_reg' is not defined

@ktsakalozos

Copy link
Copy Markdown
Contributor

@etheleon now we have:

>       output = (subprocess
                  .run(['reg', 'ls', '-f', 'localhost:32000'], stdout=subprocess.PIPE)
                  .stdout
                  .decode('utf-8')
                  .split("\n"))
E       AttributeError: 'module' object has no attribute 'run'

I should document how to run the tests locally, (they are run with pytest).

Comment thread tests/test-upgrade.py


try:
enable = microk8s_enable("reg")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you please add a timeout_insec=30 when enabling the add-on. If we do not set a timeout enabling tries for too long and tests on travis timeout.

@stale

stale Bot commented Apr 22, 2020

Copy link
Copy Markdown

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale Bot added the inactive label Apr 22, 2020
@ktsakalozos

Copy link
Copy Markdown
Contributor

Closing this PR due to inactivity.

@ktsakalozos ktsakalozos closed this Jul 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants