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

feat(snap): add initial snap packaging #127

Merged
merged 15 commits into from
Nov 11, 2021

Conversation

MonicaisHer
Copy link
Contributor

fix: #6
Signed-off-by: Mengyi [email protected]

If your build fails due to your commit message not passing the build checks, please review the guidelines here: https://github.com/edgexfoundry/device-snmp-go/blob/main/.github/Contributing.md

PR Checklist

Please check if your PR fulfills the following requirements:

  • I am not introducing a breaking change (if you are, flag in conventional commit message with BREAKING CHANGE: describing the break)
  • I am not introducing a new dependency (add notes below if you are)
  • I have added unit tests for the new feature or bug fix (if not, why?) no hardware available
  • I have fully tested (add details below) this the new feature or bug fix (if not, why?)
  • I have opened a PR for the related docs change (if not, why?) this will be done after the snap been published

Testing Instructions

Test guideline:

git clone https://github.com/MonicaisHer/device-snmp-go.git
cd device-snmp-go
git checkout add-snap-v2
snapcraft
snap install edgexfoundry --channel=2.0
snap install edgex-device-snmp_2.0.1-dev.4_amd64.snap --dangerous
sudo snap connect edgexfoundry:edgex-secretstore-token edgex-device-snmp:edgex-secretstore-token
sudo nano /var/snap/edgex-device-snmp/current/config/device-snmp/res/profiles/device.snmp.trendnet.TPE082WS.yaml

please change the deviceResources's RebootCurrentState's properties readWrite: "R" to "RW", to let device profile align the device resource. (issue: #126 )

check if the snap is running:

snap start --enable edgex-device-snmp
snap logs edgex-device-snmp

expect:

level=INFO ts=2021-10-28T11:38:56.615380966Z app=device-snmp source=message.go:55 msg="device snmp started"

ping edgex-device-snmp's port:

curl http://localhost:59993/api/v2/ping

expect:{"apiVersion":"v2","timestamp":"Wed Oct 27 14:11:36 CEST 2021"}

New Dependency Instructions (If applicable)

@MonicaisHer MonicaisHer marked this pull request as ready for review October 28, 2021 12:00
@MonicaisHer
Copy link
Contributor Author

Could you please help me review it, thanks! @farshidtz @siggiskulason @tonyespy

snap/local/hooks/go.mod Outdated Show resolved Hide resolved
snap/snapcraft.yaml Outdated Show resolved Hide resolved
snap/snapcraft.yaml Outdated Show resolved Hide resolved
snap/hooks/pre-refresh Outdated Show resolved Hide resolved
@farshidtz
Copy link
Member

Thanks @MonicaisHer. I added several inline comments. Please also add a snap/README.md.

@MonicaisHer
Copy link
Contributor Author

MonicaisHer commented Oct 28, 2021

Thanks @MonicaisHer. I added several inline comments. Please also add a snap/README.md.

Thanks for your review @farshidtz . I was intent to add readme after publishing this snap because it's never been published, so it cant be installed from either the snap store or using snap install yet by a user. If you want me to add now, should I add steps on how to build a snap by snapcraft?

@MonicaisHer MonicaisHer requested a review from farshidtz October 28, 2021 17:34
@farshidtz
Copy link
Member

Thanks for your review @farshidtz . I was intent to add readme after publishing this snap because it's never been published, so it cant be installed from either the snap store or using snap install yet by a user. If you want me to add now, should I add steps on how to build a snap by snapcraft?

That's a good point. I can think of three options:

  1. Publish manually once and then publish the code with the docs
  2. Publish the code with docs for manual build and installation steps. Once added to store, add the store badge and replace the commands.
  3. Add a separate PR with the expected README, set it to draft and link to it from here. We can merge that PR later on.

What do you think @siggiskulason @tonyespy?

@farshidtz
Copy link
Member

farshidtz commented Oct 29, 2021

The top level go.mod and go.sum are probably changed by mistake. Please remove/revert that commit and go mod tidy.

ernestojeda and others added 9 commits October 29, 2021 17:15
Signed-off-by: Ernesto Ojeda <[email protected]>
Signed-off-by: Mengyi <[email protected]>
- update base to core20
- optimize copying config files in snapcraft.yaml
- update edgex-snap-hooks to v2.0.7
- remove `GO=GO111MODULE=on go` in snap/local/hooks/Makefile
- remove snap/hooks/pre-fresh

Signed-off-by: Mengyi <[email protected]>
update `$(GO)` in snap/local/hooks/Makefile

Signed-off-by: Mengyi <[email protected]>
- add go.mod
- add go.sum

Signed-off-by: Mengyi <[email protected]>
- add go.mod
- add go.sum

Signed-off-by: Mengyi <[email protected]>
@MonicaisHer
Copy link
Contributor Author

The top level go.mod and go.sum are probably changed by mistake. Please remove/revert that commit and go mod tidy.

Thanks. That commit has been reverted.

- add snap/README.md
- update snap/local/hooks/const.go
- remove snap/local/.gitkeep

Signed-off-by: Mengyi <[email protected]>
@MonicaisHer MonicaisHer requested a review from farshidtz November 4, 2021 13:23
@keanjapesan
Copy link

recheck

@codecov-commenter
Copy link

codecov-commenter commented Nov 5, 2021

Codecov Report

Merging #127 (e0d79c5) into main (128899a) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff          @@
##            main    #127   +/-   ##
=====================================
  Coverage   0.00%   0.00%           
=====================================
  Files          2       2           
  Lines        200     200           
=====================================
  Misses       200     200           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8bb3280...e0d79c5. Read the comment docs.

snap/README.md Outdated
device-config:
interface: content
content: device-config
write:
Copy link
Member

Choose a reason for hiding this comment

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

The key should be read, not write as it is exposing the read-only $SNAP/config directory.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your re-review, I have updated this part. I also updated this part's word based on your another PR.

snap/README.md Outdated Show resolved Hide resolved
snap/README.md Outdated Show resolved Hide resolved
- update `Using a content interface to set device configuration`
- remove `Service.BootTimeout` and `Service.Protocol` in
  `Service Environment Configuration Overrides`
- update words based on https://github.com/edgexfoundry/
  device-mqtt-go/pull/311/files

Signed-off-by: Mengyi <[email protected]>
Copy link
Member

@farshidtz farshidtz left a comment

Choose a reason for hiding this comment

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

Thanks. Looks good.
Please squash and merge which should also fix the semantic commit issues.

@MonicaisHer
Copy link
Contributor Author

@siggiskulason Could you please help me squash and merge this PR? Thank you!

Copy link

@siggiskulason siggiskulason left a comment

Choose a reason for hiding this comment

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

LGTM

@siggiskulason siggiskulason merged commit 8cef907 into edgexfoundry:main Nov 11, 2021
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.

Add snap packaging
6 participants