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

Add subscription Organization field and tests #4697

Closed
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@dperpeet
Member

dperpeet commented Jul 8, 2016

  • candlepin image
  • integration test with registration, wrong password, missing org
  • add optional organization field (supported by subscription-manager)

@dperpeet dperpeet added the priority label Jul 8, 2016

# adjust database permissions
cat << xxEOFxx > /var/lib/pgsql/data/pg_hba.conf

This comment has been minimized.

@petervo

petervo Jul 8, 2016

Contributor

Maybe this is what's needed, and probably not worth spending time on, but those are pretty open permissions.

@petervo

petervo Jul 8, 2016

Contributor

Maybe this is what's needed, and probably not worth spending time on, but those are pretty open permissions.

This comment has been minimized.

@dperpeet

dperpeet Jul 8, 2016

Member

Maybe this is what's needed, and probably not worth spending time on, but those are pretty open permissions.

Yes. The goal of the candlepin image is to provide a running subscription server for us to use - it doesn't need to be secure and it only contains test data. I would say this could be optimized in a follow-up, but it's not really necessary in my opinion.

@dperpeet

dperpeet Jul 8, 2016

Member

Maybe this is what's needed, and probably not worth spending time on, but those are pretty open permissions.

Yes. The goal of the candlepin image is to provide a running subscription server for us to use - it doesn't need to be secure and it only contains test data. I would say this could be optimized in a follow-up, but it's not really necessary in my opinion.

This comment has been minimized.

@stefwalter

stefwalter Jul 11, 2016

Contributor

Makes sense.

@stefwalter

stefwalter Jul 11, 2016

Contributor

Makes sense.

# 02110-1301 USA.
BASE=$(dirname $0)
$BASE/virt-builder-fedora "$1" fedora-22 x86_64

This comment has been minimized.

@petervo

petervo Jul 8, 2016

Contributor

Is there a reason to stick to fedora 22 instead of using 23 or 24?

@petervo

petervo Jul 8, 2016

Contributor

Is there a reason to stick to fedora 22 instead of using 23 or 24?

This comment has been minimized.

@dperpeet

dperpeet Jul 8, 2016

Member

Sadly the candlepin docs use fedora 22 and my attempts to use 23 or 24 failed. I didn't want to migrate their setup to a newer fedora.

@dperpeet

dperpeet Jul 8, 2016

Member

Sadly the candlepin docs use fedora 22 and my attempts to use 23 or 24 failed. I didn't want to migrate their setup to a newer fedora.

This comment has been minimized.

@stefwalter

stefwalter Jul 11, 2016

Contributor

Makes sense.

@stefwalter

stefwalter Jul 11, 2016

Contributor

Makes sense.

@@ -433,6 +440,9 @@ require([
} else if (buffer.indexOf('Unable to reach the server at') === 0) {
reason = "server";
message = buffer.trim();
} else if ((buffer.indexOf('EOF') === 0) && (buffer.indexOf('Organization: ') !== -1)) {

This comment has been minimized.

@petervo

petervo Jul 8, 2016

Contributor

More out of curiosity, why is the EOF check needed here but not on the others.

@petervo

petervo Jul 8, 2016

Contributor

More out of curiosity, why is the EOF check needed here but not on the others.

This comment has been minimized.

@dperpeet

dperpeet Jul 8, 2016

Member

The error message literally begins with the string EOF, since we use CLI scraping for this and subscription-manager wants to have user input. As a second condition, we want to see if it asks for our organization. I think the complete message contains some other stuff I wasn't comfortable with checking against in its entirety.

@dperpeet

dperpeet Jul 8, 2016

Member

The error message literally begins with the string EOF, since we use CLI scraping for this and subscription-manager wants to have user input. As a second condition, we want to see if it asks for our organization. I think the complete message contains some other stuff I wasn't comfortable with checking against in its entirety.

@petervo petervo added the question label Jul 8, 2016

@dperpeet dperpeet removed the question label Jul 8, 2016

@petervo

This comment has been minimized.

Show comment
Hide comment
@petervo

petervo Jul 8, 2016

Contributor

I'm ok with this, would like to see the tests pass first.

Contributor

petervo commented Jul 8, 2016

I'm ok with this, would like to see the tests pass first.

@dperpeet

This comment has been minimized.

Show comment
Hide comment
@dperpeet

dperpeet Jul 8, 2016

Member

I'm ok with this, would like to see the tests pass first.

Thanks. Definitely! They should only run on rhel-7, but I would make sure that they pass there and aren't run on fedora-24 and rhel-atomic.

Member

dperpeet commented Jul 8, 2016

I'm ok with this, would like to see the tests pass first.

Thanks. Definitely! They should only run on rhel-7, but I would make sure that they pass there and aren't run on fedora-24 and rhel-atomic.

@petervo petervo added the needswork label Jul 9, 2016

Show outdated Hide outdated test/candlepin Outdated
@petervo

This comment has been minimized.

Show comment
Hide comment
@petervo

petervo Jul 9, 2016

Contributor

Test failed, I think the updated image link is in the wrong place.

Contributor

petervo commented Jul 9, 2016

Test failed, I think the updated image link is in the wrong place.

@dperpeet

This comment has been minimized.

Show comment
Hide comment
@dperpeet

dperpeet Jul 9, 2016

Member

Test failed, I think the updated image link is in the wrong place.

Well, good catch with the (extra) wrong link. The right link is in the right place, but I forgot to add candlepin to testsuite-prepare. I'll merge #4680 first.

Member

dperpeet commented Jul 9, 2016

Test failed, I think the updated image link is in the wrong place.

Well, good catch with the (extra) wrong link. The right link is in the right place, but I forgot to add candlepin to testsuite-prepare. I'll merge #4680 first.

@dperpeet dperpeet removed the needswork label Jul 9, 2016

@dperpeet

This comment has been minimized.

Show comment
Hide comment
@dperpeet

dperpeet Jul 9, 2016

Member

removed erroneous link and rebased on #4680

Member

dperpeet commented Jul 9, 2016

removed erroneous link and rebased on #4680

@stefwalter

This comment has been minimized.

Show comment
Hide comment
@stefwalter

stefwalter Jul 9, 2016

Contributor
Wrote TestSubscriptions-testRegister-FAIL.png
Journal extracted to TestSubscriptions-testRegister-10.111.123.104-FAIL.log
Journal extracted to TestSubscriptions-testRegister-10.111.114.82-FAIL.log
not ok 97 testRegister (check_subscriptions.TestSubscriptions)
Traceback (most recent call last):
  File "./verify/check-subscriptions", line 107, in testRegister
    b.wait_present(product_selector)
  File "/build/cockpit/test/common/testlib.py", line 217, in wait_present
    return self.wait_js_func('ph_is_present', selector)
  File "/build/cockpit/test/common/testlib.py", line 211, in wait_js_func
    return self.phantom.wait("%s(%s)" % (func, ','.join(map(jsquote, args))))
  File "/build/cockpit/test/common/testlib.py", line 665, in <lambda>
    return lambda *args: self._invoke(name, *args)
  File "/build/cockpit/test/common/testlib.py", line 688, in _invoke
    raise Error(res['error'])
Error: timeout
Contributor

stefwalter commented Jul 9, 2016

Wrote TestSubscriptions-testRegister-FAIL.png
Journal extracted to TestSubscriptions-testRegister-10.111.123.104-FAIL.log
Journal extracted to TestSubscriptions-testRegister-10.111.114.82-FAIL.log
not ok 97 testRegister (check_subscriptions.TestSubscriptions)
Traceback (most recent call last):
  File "./verify/check-subscriptions", line 107, in testRegister
    b.wait_present(product_selector)
  File "/build/cockpit/test/common/testlib.py", line 217, in wait_present
    return self.wait_js_func('ph_is_present', selector)
  File "/build/cockpit/test/common/testlib.py", line 211, in wait_js_func
    return self.phantom.wait("%s(%s)" % (func, ','.join(map(jsquote, args))))
  File "/build/cockpit/test/common/testlib.py", line 665, in <lambda>
    return lambda *args: self._invoke(name, *args)
  File "/build/cockpit/test/common/testlib.py", line 688, in _invoke
    raise Error(res['error'])
Error: timeout

@dperpeet dperpeet added the needswork label Jul 9, 2016

@dperpeet

This comment has been minimized.

Show comment
Hide comment
@dperpeet

dperpeet Jul 9, 2016

Member

This might be an actual bug where the page doesn't refresh properly after subscribing... I will look into it.

Member

dperpeet commented Jul 9, 2016

This might be an actual bug where the page doesn't refresh properly after subscribing... I will look into it.

Show outdated Hide outdated test/verify/files/6050.pem Outdated

@stefwalter stefwalter changed the title from subscriptions: Add integration test to Add subscription Organization field and tests Jul 11, 2016

Show outdated Hide outdated pkg/subscriptions/index.html Outdated

@stefwalter stefwalter removed the needswork label Jul 11, 2016

@andreasn

This comment has been minimized.

Show comment
Hide comment
@andreasn

andreasn Jul 11, 2016

Contributor

The organization field is not mandatory, right? If so, we could have the placeholder saying "Optional"
What is multi-tenant? Is there an easier, more straight forward way of saying that?

There could be a info (i) at the very right end of the input saying something like "This is needed for subscribing if you have a setup that looks like blah blah"

Contributor

andreasn commented Jul 11, 2016

The organization field is not mandatory, right? If so, we could have the placeholder saying "Optional"
What is multi-tenant? Is there an easier, more straight forward way of saying that?

There could be a info (i) at the very right end of the input saying something like "This is needed for subscribing if you have a setup that looks like blah blah"

@dperpeet

This comment has been minimized.

Show comment
Hide comment
@dperpeet

dperpeet Jul 11, 2016

Member

The organization field is not mandatory, right? If so, we could have the placeholder saying "Optional"
What is multi-tenant? Is there an easier, more straight forward way of saying that?

I think the best explanation for this field is "if you don't know what it means, you don't need it" :)

There could be a info (i) at the very right end of the input saying something like "This is needed for subscribing if you have a setup that looks like blah blah"

Difficult to say. Some servers want it, others don't. The official Red Hat server doesn't need it, for example. But there are on-site servers where multiple organizations share their own server, e.g. when 'Acme Inc.' and 'Software Ltd.' share office space and a single server.

@andreasn should I just leave out the placeholder text? or optional in many cases?

Member

dperpeet commented Jul 11, 2016

The organization field is not mandatory, right? If so, we could have the placeholder saying "Optional"
What is multi-tenant? Is there an easier, more straight forward way of saying that?

I think the best explanation for this field is "if you don't know what it means, you don't need it" :)

There could be a info (i) at the very right end of the input saying something like "This is needed for subscribing if you have a setup that looks like blah blah"

Difficult to say. Some servers want it, others don't. The official Red Hat server doesn't need it, for example. But there are on-site servers where multiple organizations share their own server, e.g. when 'Acme Inc.' and 'Software Ltd.' share office space and a single server.

@andreasn should I just leave out the placeholder text? or optional in many cases?

@andreasn

This comment has been minimized.

Show comment
Hide comment
@andreasn

andreasn Jul 11, 2016

Contributor

I think if we don't have any placeholder text, it will seem like a mandatory field to fill in order to successfully subscribe any system.
optional in many cases feels slightly vague, but it could work.

Could it work to gray out the "Organization" input if the URL field is set to "Default"? (since RH servers don't need it)

Contributor

andreasn commented Jul 11, 2016

I think if we don't have any placeholder text, it will seem like a mandatory field to fill in order to successfully subscribe any system.
optional in many cases feels slightly vague, but it could work.

Could it work to gray out the "Organization" input if the URL field is set to "Default"? (since RH servers don't need it)

@dperpeet

This comment has been minimized.

Show comment
Hide comment
@dperpeet

dperpeet Jul 11, 2016

Member

Could it work to gray out the "Organization" input if the URL field is set to "Default"? (since RH servers don't need it)

That would imply you need it with a custom server, which you might not. You don't really know until you've hit the button and actually tried it, sadly.

Member

dperpeet commented Jul 11, 2016

Could it work to gray out the "Organization" input if the URL field is set to "Default"? (since RH servers don't need it)

That would imply you need it with a custom server, which you might not. You don't really know until you've hit the button and actually tried it, sadly.

@andreasn

This comment has been minimized.

Show comment
Hide comment
@andreasn

andreasn Jul 11, 2016

Contributor

That would imply you need it with a custom server, which you might not.

You would still have the optional in many cases label to guide you though.

Contributor

andreasn commented Jul 11, 2016

That would imply you need it with a custom server, which you might not.

You would still have the optional in many cases label to guide you though.

@dperpeet

This comment has been minimized.

Show comment
Hide comment
@dperpeet

dperpeet Jul 11, 2016

Member

You would still have the optional in many cases label to guide you though.

I'll go with that text for now.

Member

dperpeet commented Jul 11, 2016

You would still have the optional in many cases label to guide you though.

I'll go with that text for now.

@dperpeet dperpeet added the needswork label Jul 11, 2016

dperpeet added some commits Jul 5, 2016

subscriptions: Add organization entry
In some setups, registering with a subscription server requires the
org info, especially in locally managed multi-tenant business units.
subscriptions: Pass args individually
Don't composite arguments to subscription-manager with =, but pass
them as separate args. E.g: '--username user' instead of
'--username=user'.

@dperpeet dperpeet removed the needswork label Jul 11, 2016

@dperpeet

This comment has been minimized.

Show comment
Hide comment
@dperpeet

dperpeet Jul 11, 2016

Member

After irc discussion with @andreasn I've changed the organization placeholder text to usually optional.

Member

dperpeet commented Jul 11, 2016

After irc discussion with @andreasn I've changed the organization placeholder text to usually optional.

stefwalter added a commit that referenced this pull request Jul 11, 2016

test: Add candlepin integration test
Closes #4697
Reviewed-by: Stef Walter <stefw@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment