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

snap: update for new snapcraft, fix old bugs #617

Conversation

anonymouse64
Copy link
Contributor

@anonymouse64 anonymouse64 commented Jul 13, 2020

This PR fixes the broken CI jobs for edgex snaps since upstream snapcraft now requires to use snap pack instead of mksquashfs. There was not an issue filed for this, perhaps @tonyespy can file one if needed.

It also fixes the long standing issue #369 by installing the expect package in the docker container used to build the snap and using the associated unbuffer command to prevent output from getting mangled by docker stdout streaming.

Finally, it also fixes other issues we have had where if the download for the core, core18, or snapcraft snaps fail and the hash doesn't match, we now fail the build much earlier using && instead of ; between commands. An issue for this type of issue was filed at #517.

This can be tested by checking out master branch of edgex-go and running the edgexfoundry-snapcraft.sh script from this repo from inside the edgex-go repo, i.e.

$ git clone https://github.com/edgexfoundry/edgex-go.git
$ cd edgex-go
$ # git clone this PR
$ /home/user/git/ci-management/shell/edgexfoundry-snapcraft.sh 
... should produce working output and no snapcraft error trace like we see in https://github.com/edgexfoundry/edgex-go/pull/2613#issuecomment-657112589

Not sure what labels to use for this PR, the PR labels are not documented in https://github.com/edgexfoundry/ci-management/blob/master/.github/CONTRIBUTING.md

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Documentation content changes
  • Other... Please describe:

Issue Number:

Sandbox Testing

Test Links :

Didn't test in the sandbox since it can be tested locally as mentioned above. I would have to dig up my old instructions on how to do that before I can test in the sandbox

The previous behavior would not actually fail the build setup if for example the
SHA's didn't match, but now if the SHA's don't match, sha512sum will exit
non-zero and using && results in the whole command failing.

We also should delete the sha512 file as well, it is not needed during the
build.

Fixes: edgexfoundry#517

Signed-off-by: Ian Johnson <[email protected]>
This is needed because now snapcraft will use "snap pack" from the host, and so
we need the snap command in the docker container alongside the snapcraft snap.

Signed-off-by: Ian Johnson <[email protected]>
Due to some libraries that snapcraft/python use for outputting, the ordering of
some output gets lost when it is piped through docker. To fix this we can just
run snapcraft through unbuffer, which is provided through the expect package.

Fixes: edgexfoundry#369

Signed-off-by: Ian Johnson <[email protected]>
@anonymouse64 anonymouse64 changed the title Feature/update for new snapcraft snap: update for new snapcraft, fix old bugs Jul 13, 2020
Copy link
Member

@eb-oss eb-oss left a comment

Choose a reason for hiding this comment

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

Can you squash the 3 commits down to one?

@anonymouse64
Copy link
Contributor Author

@MightyNerdEric the individual commits fix different issues, would you rather I submit 3 separate PR's ?

@ernestojeda
Copy link
Contributor

@anonymouse64 These scripts are no longer used to push snaps. All snap scripts have been moved to ci-build-images. You can find the corresponding script/docker image here:

https://github.com/edgexfoundry/ci-build-images/blob/snapcraft-builder/Dockerfile
https://github.com/edgexfoundry/ci-build-images/blob/snapcraft-builder/entrypoint.sh

Are you able to make those changes in in the ci-build-images repo?

@ernestojeda ernestojeda self-requested a review July 13, 2020 20:45
Copy link
Contributor

@ernestojeda ernestojeda left a comment

Choose a reason for hiding this comment

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

See my comment. I will work to remove these files to avoid the confusion.

Copy link
Member

@tonyespy tonyespy left a comment

Choose a reason for hiding this comment

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

Tested build, which now works, and reviewed the code changes.

@eb-oss eb-oss self-requested a review July 13, 2020 21:20
@anonymouse64
Copy link
Contributor Author

@ernestojeda how does one test the changes to that repo locally without using the sandbox? I'm happy to submit the changes there instead but I don't know how the pipelines jobs work so I don't know how to reproduce what jenkins does locally with that repo

@ernestojeda
Copy link
Contributor

@ernestojeda how does one test the changes to that repo locally without using the sandbox? I'm happy to submit the changes there instead but I don't know how the pipelines jobs work so I don't know how to reproduce what jenkins does locally with that repo

@anonymouse64 Yeah, it will be pretty much impossible to test this locally, but It is definitely possible to test this on the sandbox. However, it will be pretty difficult to pull off. If it's OK with you, would you mind if the DevOps team took these changes over and integrated them into the ci-build-images repository? We could then demo this in an upcoming WG meeting to show the testing process.

@anonymouse64
Copy link
Contributor Author

@ernestojeda sounds fine to me, only thing would be that all the snap PR jobs are currently broken, so if you could at least commit / merge 7c46d5a ASAP that would be appreciated. The other commits are not needed ASAP so are fine to re-use for demo purposes, etc.

With that in mind do you still want me to submit PR's against ci-build-images or will you take care of that?

@ernestojeda
Copy link
Contributor

I will take care of that.

ernestojeda added a commit to ernestojeda/ci-build-images that referenced this pull request Jul 14, 2020
Migrated from ci-management: edgexfoundry/ci-management#617
Changes by Ian Johnson <[email protected]>

Due to some libraries that snapcraft/python use for outputting, the ordering of
some output gets lost when it is piped through docker. To fix this we can just
run snapcraft through unbuffer, which is provided through the expect package.

This is needed because now snapcraft will use "snap pack" from the host, and so
we need the snap command in the docker container alongside the snapcraft snap.

The previous behavior would not actually fail the build setup if for example the
SHA's didn't match, but now if the SHA's don't match, sha512sum will exit
non-zero and using && results in the whole command failing.

We also should delete the sha512 file as well, it is not needed during the
build.

Fixes: #517

Signed-off-by: Ernesto Ojeda <[email protected]>
ernestojeda added a commit to ernestojeda/ci-build-images that referenced this pull request Jul 14, 2020
Migrated from ci-management: edgexfoundry/ci-management#617
Changes by Ian Johnson <[email protected]>

Due to some libraries that snapcraft/python use for outputting, the ordering of
some output gets lost when it is piped through docker. To fix this we can just
run snapcraft through unbuffer, which is provided through the expect package.

This is needed because now snapcraft will use "snap pack" from the host, and so
we need the snap command in the docker container alongside the snapcraft snap.

The previous behavior would not actually fail the build setup if for example the
SHA's didn't match, but now if the SHA's don't match, sha512sum will exit
non-zero and using && results in the whole command failing.

We also should delete the sha512 file as well, it is not needed during the
build.

Fixes: #517

Signed-off-by: Ernesto Ojeda <[email protected]>
@anonymouse64 anonymouse64 deleted the feature/update-for-new-snapcraft branch August 26, 2020 03:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants