CCI-MOC / hil

Hardware Isolation Layer, formerly Hardware as a Service
Apache License 2.0
24 stars 54 forks source link

Make OBMd connection info required. #991

Closed zenhack closed 6 years ago

zenhack commented 6 years ago

This is built on top of #990, which should be merged first. Only the most recent commit is specific to this PR; see the commit message there for details.

We should wait until 0.3 has been tagged to merge this, since it involves changes that can't go into effect until after users have had the change to run our migration helper script.

We should also run the deployment tests for this one. I made an attempt at updating the deployment tests with the relevant changes (site-layout.json now needs to include the obmd info as well), but this needs testing.

Also, I noticed while I was editing the example site-layout.json that it's format seems to differ from what the code wants; it has a top-level "ipmi" field, but the code references an "obm" field. I have a hunch that this is bitrotten -- @naved001, can you compare against the site-layout.json that we've been using?

zenhack commented 6 years ago

Suspicions about site-layout.json were correct. I pushed a fix for the depolyment tests. It looks like there may also be some failures in the migration tests; will investigate more soon.

zenhack commented 6 years ago

Still tracking down some places where stuff needed to change, but should be working soon. This is ready for review; there aren't going to be any big structural changes.

The amount of time a type system would have saved me on this one is insane.

zenhack commented 6 years ago

Ok, CI is passing.

naved001 commented 6 years ago

this can be rebased now

zenhack commented 6 years ago

@naved001, done.

coveralls commented 6 years ago

Pull Request Test Coverage Report for Build 1839


Changes Missing Coverage Covered Lines Changed/Added Lines %
hil/client/node.py 0 2 0.0%
hil/model.py 0 2 0.0%
hil/api.py 0 3 0.0%
hil/cli/node.py 0 3 0.0%
hil/test_common.py 0 3 0.0%
<!-- Total: 0 13 0.0% -->
Files with Coverage Reduction New Missed Lines %
hil/model.py 1 0.0%
hil/client/node.py 3 0.0%
<!-- Total: 4 -->
Totals Coverage Status
Change from base Build 1832: 0.0%
Covered Lines: 0
Relevant Lines: 2580

💛 - Coveralls
zenhack commented 6 years ago

Travis failure seem to be because of a new pycodestyle release, as of today; everything is explained by the release notes:

https://pypi.python.org/pypi/pycodestyle/2.4.0

I can fix these, but it might be better to do so separately, as unrelated style changes will make the diff harder to follow.

naved001 commented 6 years ago

I can fix these, but it might be better to do so separately, as unrelated style changes will make the diff harder to follow.

After this PR is approved, you could push one last commit to fix pylint errors before we merge it.

Besides the pylint errors, there are a bunch of line-too-long errors.

Edit: Dont know why I kept thinking it was pylint that got upgraded.

zenhack commented 6 years ago

I think the errors are from pycodestyle, not pylint, but sounds good. The line too long errors are for bits of docstring that were already there; see the release notes, apparently there was a bug where it was a bit more lax before.

Quoting Naved Ansari (2018-04-11 10:23:14)

 I can fix these, but it might be better to do so separately, as
 unrelated style changes will make the diff harder to follow.

After this PR is approved, you could push one last commit to fix pylint errors before we merge it.

Besides the pylint errors, there are a bunch of line-too-long errors.

-- You are receiving this because you were assigned. Reply to this email directly, [1]view it on GitHub, or [2]mute the thread.

Verweise

  1. https://github.com/CCI-MOC/hil/pull/991#issuecomment-380470514
  2. https://github.com/notifications/unsubscribe-auth/AA18Prawp4mbds-w8_GpeIxrb_l9ZIE8ks5tnhHSgaJpZM4TLVHs
zenhack commented 6 years ago

@xuhang57, yeah, obmd will be a hard dependency as of 0.4. This is why we made a point of getting another release out (0.3) before then, with the necessary stuff to upgrade. 0.3 has already been tagged and released, so I'm not sure exactly what you're suggesting?

xuhang57 commented 6 years ago

@zenhack I was thinking of creating a new branch based on 0.3, but it seems unnecessary since we have tagged/released 0.3 version?

The only advantage that I could think of about having a 0.3 branch is that someone who does not nee obmd can use HIL by switching the branch. Not sure whether it makes sense or not.

zenhack commented 6 years ago

Quoting Lucas H. Xu (2018-04-16 21:27:27)

The only advantage that I could think of about having a 0.3 branch is that someone who does not nee obmd can use HIL by switching the branch. Not sure whether it makes sense or not.

You can check out tags too. I don't think there's any need for a branch.

ianballou commented 6 years ago

Not a big deal enough for me to take the check away, but there might be some documentation that'll need to change, like at the bottom of testing.md for example.

zenhack commented 6 years ago

I pushed another commit to fix most of the pycodestyle errors. Per the commit message I left a few of them out, which are in regexes for the drivers -- if not done carefully we could introduce breakage, so I think it's worth deferring to a separate pr.

In general we should be using raw strings for regexes, so we don't have to think about two levels of escape sequences.

naved001 commented 6 years ago

I don't see the regexes in the failures? I think we are just concatenating strings which happen to contain a backslash. And I don't think we can change that. We should probably disable that check in those lines.

jeremyfreudberg commented 6 years ago

Switching to so-called raw strings should fix it. The error message for W605 should be clearer, but it means to say that because "\(" is not an actual escape sequence, the backslash itself needs to be escaped (otherwise you are relying on an implementation detail that "invalid" escape sequences aren't interpolated, rather than cause an error)