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

build_providers: add ssh key managemet to the qemu build provider #2168

Merged
merged 4 commits into from Jun 26, 2018

Conversation

sergiusens
Copy link
Collaborator

…ider

LP: #1774013
Signed-off-by: Sergio Schvezov sergio.schvezov@canonical.com

  • Have you followed the guidelines for contributing?
  • Have you signed the CLA?
  • If this is a bugfix. Have you checked that there is a bug report open for the issue you are trying to fix on bug reports?
  • If this is a new feature. Have you discussed the design on the forum?
  • Have you successfully run ./runtests.sh static?
  • Have you successfully run ./runtests.sh unit?

@sergiusens sergiusens changed the title build_providers: add ssh key managemet support to the qemu build prov… build_providers: add ssh key managemet to the qemu build provider Jun 25, 2018
…ider

LP: #1774013
Signed-off-by: Sergio Schvezov <sergio.schvezov@canonical.com>
@codecov-io
Copy link

codecov-io commented Jun 25, 2018

Codecov Report

Merging #2168 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #2168      +/-   ##
=========================================
+ Coverage   91.29%   91.3%   +<.01%     
=========================================
  Files         200     201       +1     
  Lines       12580   12605      +25     
  Branches     1871    1872       +1     
=========================================
+ Hits        11485   11509      +24     
  Misses        741     741              
- Partials      354     355       +1
Impacted Files Coverage Δ
snapcraft/internal/build_providers/errors.py 100% <100%> (ø) ⬆️
snapcraft/internal/build_providers/_qemu/_keys.py 100% <100%> (ø)
snapcraft/internal/elf.py 82.84% <0%> (-0.33%) ⬇️

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 4f80a10...ece5427. Read the comment docs.

Copy link
Contributor

@kyrofa kyrofa left a comment

Choose a reason for hiding this comment

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

A few comments, but nothing at all major.

SSHKeyT = TypeVar('SSHKeyT', bound='SSHKey')


class SSHKeyPathError(SnapcraftError):
Copy link
Contributor

Choose a reason for hiding this comment

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

This error class name could be more descriptive: it's not just a problem with the path (permissions etc.), it's that the path doesn't actually exist.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, no strong feelings here, but I'm curious why this isn't in build_providers/errors.py?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I had the feeling it was qemu specific, but after a check I see the telnet stuff made it there too.

super().__init__(private_key_file_path=private_key_file_path)


class SSHKey:
Copy link
Contributor

Choose a reason for hiding this comment

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

Class and function docs would be nice.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

hmm, I wonder where they went, I did actually write them 😕

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

reflog did not rescue me :-/

Copy link
Contributor

@kyrofa kyrofa left a comment

Choose a reason for hiding this comment

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

Looks good, thank you!

@sergiusens sergiusens merged commit 208a767 into canonical:master Jun 26, 2018
@sergiusens sergiusens deleted the ssh-keys branch June 26, 2018 17:09
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

3 participants