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

Debian #3202

Closed
wants to merge 36 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@mvollmer
Member

mvollmer commented Nov 20, 2015

For later:

  • Enable storage #3295
  • Enable networking #1308, #3341
  • PCP #3294
  • Take user-synch dialog out of machine-adding flow. #3293
  • Allow patching of files that we import via bower. #3297
  • Figure out build-deps from the source package instead of listing them explicitly. #3298
@mvollmer

This comment has been minimized.

Show comment
Hide comment
@mvollmer

mvollmer Nov 20, 2015

Member

@mbiebl, fyi. I guess our TODOs overlap a bit, let's coordinate!

Member

mvollmer commented Nov 20, 2015

@mbiebl, fyi. I guess our TODOs overlap a bit, let's coordinate!

@mvollmer

This comment has been minimized.

Show comment
Hide comment
@mvollmer

mvollmer Nov 20, 2015

Member

./vm-create never completes. It seems to miss the shutdown event somehow...

Member

mvollmer commented Nov 20, 2015

./vm-create never completes. It seems to miss the shutdown event somehow...

@mvollmer

This comment has been minimized.

Show comment
Hide comment
@mvollmer
Member

mvollmer commented Nov 20, 2015

@fsateler

This comment has been minimized.

Show comment
Hide comment
@fsateler

fsateler Nov 23, 2015

@mvollmer The debian/*.debhelper.log files are build artifacts, they should not be commited (they are more or less equivalent to a make stamp file).

Also, it probably would be a good idea to run the CI with verbose output so that we can see the pbuilder output.

fsateler commented Nov 23, 2015

@mvollmer The debian/*.debhelper.log files are build artifacts, they should not be commited (they are more or less equivalent to a make stamp file).

Also, it probably would be a good idea to run the CI with verbose output so that we can see the pbuilder output.

@mvollmer

This comment has been minimized.

Show comment
Hide comment
@mvollmer

mvollmer Nov 23, 2015

Member

@mvollmer The debian/*.debhelper.log files are build artifacts, they should not be commited (they are more or less equivalent to a make stamp file).

Ouch, I knew that! :-) Thanks!

Member

mvollmer commented Nov 23, 2015

@mvollmer The debian/*.debhelper.log files are build artifacts, they should not be commited (they are more or less equivalent to a make stamp file).

Ouch, I knew that! :-) Thanks!

@mvollmer

This comment has been minimized.

Show comment
Hide comment
@mvollmer

mvollmer Nov 23, 2015

Member

The debian/*.debhelper.log files are build artifacts, they should not be commited (they are more or less equivalent to a make stamp file).

Fixed.

Member

mvollmer commented Nov 23, 2015

The debian/*.debhelper.log files are build artifacts, they should not be commited (they are more or less equivalent to a make stamp file).

Fixed.

@mvollmer

This comment has been minimized.

Show comment
Hide comment
@mvollmer

mvollmer Nov 23, 2015

Member

Also, it probably would be a good idea to run the CI with verbose output so that we can see the pbuilder output.

The output is here (for example): https://mvo.fedorapeople.org/logs/test-1ecba5d1-debian-8-x86_64/build-results/build.log

It's a bit hard to find. Click on "Result directory" at the top of log.html and then look into build-results/.

Member

mvollmer commented Nov 23, 2015

Also, it probably would be a good idea to run the CI with verbose output so that we can see the pbuilder output.

The output is here (for example): https://mvo.fedorapeople.org/logs/test-1ecba5d1-debian-8-x86_64/build-results/build.log

It's a bit hard to find. Click on "Result directory" at the top of log.html and then look into build-results/.

@mbiebl

This comment has been minimized.

Show comment
Hide comment
@mbiebl

mbiebl Nov 23, 2015

Contributor

Hi @mvollmer, I see you are building directly from Git and you run autoreconf.
In such situations, it's usually recommended to use dh-autoreconf, which will keep track of the autogenerated "stuff" and properly remove it again on clean.

That aside, a Git build currently requires network access, as the build system installs npm modules during the build. Our Debian buildds don't necessarily have network access and it's a policy violation if the build requires network access. A package in Debian has to be compilable without external resources. This means, all the JavaScript libraries/tools would have to be packaged for Debian.

Contributor

mbiebl commented Nov 23, 2015

Hi @mvollmer, I see you are building directly from Git and you run autoreconf.
In such situations, it's usually recommended to use dh-autoreconf, which will keep track of the autogenerated "stuff" and properly remove it again on clean.

That aside, a Git build currently requires network access, as the build system installs npm modules during the build. Our Debian buildds don't necessarily have network access and it's a policy violation if the build requires network access. A package in Debian has to be compilable without external resources. This means, all the JavaScript libraries/tools would have to be packaged for Debian.

@fsateler

This comment has been minimized.

Show comment
Hide comment
@fsateler

fsateler Nov 23, 2015

@mvollmer the vm needs to have openssh-client installed if it wants to use ssh-keygen. That should fix 2 test failures. The remaining one seems to be about not having non-loopback interfaces. I can reproduce the failure using systemd-nspawn --private-network -u $myuser -D chroot make -C $(pwd) check VERBOSE=1 (after creating a proper chroot tree inside chroot dir and bind mounting the cockpit source there).

fsateler commented Nov 23, 2015

@mvollmer the vm needs to have openssh-client installed if it wants to use ssh-keygen. That should fix 2 test failures. The remaining one seems to be about not having non-loopback interfaces. I can reproduce the failure using systemd-nspawn --private-network -u $myuser -D chroot make -C $(pwd) check VERBOSE=1 (after creating a proper chroot tree inside chroot dir and bind mounting the cockpit source there).

@mvollmer

This comment has been minimized.

Show comment
Hide comment
@mvollmer

mvollmer Nov 23, 2015

Member

the vm needs to have openssh-client installed if it wants to use ssh-keygen.

Thanks! The tests run inside pbuilder, so openssh-client needs to be in Build-Depends, right?

Member

mvollmer commented Nov 23, 2015

the vm needs to have openssh-client installed if it wants to use ssh-keygen.

Thanks! The tests run inside pbuilder, so openssh-client needs to be in Build-Depends, right?

@fsateler

This comment has been minimized.

Show comment
Hide comment
@fsateler

fsateler Nov 23, 2015

Correct, it should be added to Build-Depends.

I have the following backtrace for the error:

#0  0x00007ffff754988b in g_logv () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
#1  0x00007ffff75499ff in g_log () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
#2  0x00007ffff7ac20ba in g_network_address_parse () from /usr/lib/x86_64-linux-gnu/libgio-2.0.so.0
#3  0x00007ffff7ad85f0 in g_socket_client_connect_to_host_async () from /usr/lib/x86_64-linux-gnu/libgio-2.0.so.0
#4  0x0000000000406eb4 in perform_http_request (hostport=0x0, request=request@entry=0x411000 "GET /pkg/shell/index.html HTTP/1.0\r\nHost:test\r\n\r\n", length=length@entry=0x0)
    at src/common/test-webserver.c:363
#5  0x00000000004075fa in test_webserver_noredirect_exception (tc=0x65ed90, data=<optimized out>) at src/common/test-webserver.c:527
#6  0x00007ffff756857b in ?? () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
#7  0x00007ffff7568743 in ?? () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
#8  0x00007ffff756894e in g_test_run_suite () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
#9  0x00007ffff7568971 in g_test_run () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
#10 0x0000000000406d47 in main (argc=3, argv=0x7fffffffece8) at src/common/test-webserver.c:741

So, either the test should be skipped when no non-loopback addresses are found, or it should not use tc->hostport if I'm reading setup correctly.

fsateler commented Nov 23, 2015

Correct, it should be added to Build-Depends.

I have the following backtrace for the error:

#0  0x00007ffff754988b in g_logv () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
#1  0x00007ffff75499ff in g_log () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
#2  0x00007ffff7ac20ba in g_network_address_parse () from /usr/lib/x86_64-linux-gnu/libgio-2.0.so.0
#3  0x00007ffff7ad85f0 in g_socket_client_connect_to_host_async () from /usr/lib/x86_64-linux-gnu/libgio-2.0.so.0
#4  0x0000000000406eb4 in perform_http_request (hostport=0x0, request=request@entry=0x411000 "GET /pkg/shell/index.html HTTP/1.0\r\nHost:test\r\n\r\n", length=length@entry=0x0)
    at src/common/test-webserver.c:363
#5  0x00000000004075fa in test_webserver_noredirect_exception (tc=0x65ed90, data=<optimized out>) at src/common/test-webserver.c:527
#6  0x00007ffff756857b in ?? () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
#7  0x00007ffff7568743 in ?? () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
#8  0x00007ffff756894e in g_test_run_suite () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
#9  0x00007ffff7568971 in g_test_run () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
#10 0x0000000000406d47 in main (argc=3, argv=0x7fffffffece8) at src/common/test-webserver.c:741

So, either the test should be skipped when no non-loopback addresses are found, or it should not use tc->hostport if I'm reading setup correctly.

@stefwalter

This comment has been minimized.

Show comment
Hide comment
@stefwalter

stefwalter Nov 23, 2015

Contributor

Hi @mvollmer, I see you are building directly from Git and you run autoreconf.
In such situations, it's usually recommended to use dh-autoreconf, which will keep track of the autogenerated "stuff" and properly remove it again on clean.

Yes, the debian build files need to have two slightly different 'modes':

  1. For building from git (used for testing pull requests)
  2. For building from a tarball (used for releases).

The latter should not do any network access. See the cockpit.spec file.

That aside, a Git build currently requires network access, as the build system installs npm modules during the build. Our Debian buildds don't necessarily have network access and it's a policy violation if the build requires network access. A package in Debian has to be compilable without external resources. This means, all the JavaScript libraries/tools would have to be packaged for Debian.

None of the npm packages (or even autoconf/automake) are required when doing a tarball build. The weekly releases should come from tarball builds.

Contributor

stefwalter commented Nov 23, 2015

Hi @mvollmer, I see you are building directly from Git and you run autoreconf.
In such situations, it's usually recommended to use dh-autoreconf, which will keep track of the autogenerated "stuff" and properly remove it again on clean.

Yes, the debian build files need to have two slightly different 'modes':

  1. For building from git (used for testing pull requests)
  2. For building from a tarball (used for releases).

The latter should not do any network access. See the cockpit.spec file.

That aside, a Git build currently requires network access, as the build system installs npm modules during the build. Our Debian buildds don't necessarily have network access and it's a policy violation if the build requires network access. A package in Debian has to be compilable without external resources. This means, all the JavaScript libraries/tools would have to be packaged for Debian.

None of the npm packages (or even autoconf/automake) are required when doing a tarball build. The weekly releases should come from tarball builds.

@mbiebl

This comment has been minimized.

Show comment
Hide comment
@mbiebl

mbiebl Nov 23, 2015

Contributor

there is a slight complication to that: What happens if we have to patch the non-bundled .js files from the tarball? Say for a security issue in angular, jquery, etc.

Contributor

mbiebl commented Nov 23, 2015

there is a slight complication to that: What happens if we have to patch the non-bundled .js files from the tarball? Say for a security issue in angular, jquery, etc.

@mvollmer

This comment has been minimized.

Show comment
Hide comment
@mvollmer

mvollmer Nov 23, 2015

Member

So, either the test should be skipped when no non-loopback addresses are found, or it should not use tc->hostport if I'm reading setup correctly.

It should be skipped, just like /web-server/redirect-notls.

The VM should have a non-loopback address. Does pbuilder hide it during the build? That would be a nice feature.

Member

mvollmer commented Nov 23, 2015

So, either the test should be skipped when no non-loopback addresses are found, or it should not use tc->hostport if I'm reading setup correctly.

It should be skipped, just like /web-server/redirect-notls.

The VM should have a non-loopback address. Does pbuilder hide it during the build? That would be a nice feature.

@fsateler

This comment has been minimized.

Show comment
Hide comment
@fsateler

fsateler Nov 23, 2015

Pbuilder by default enters (on linux) a new network namespace and only brings up the loopback address there.

This can be disabled setting USENETWORK=yes in /etc/pbuilderrc

I'm wondering, given that the test is already running in a VM, in this case would it be better to disable this feature, so that more tests are exercised?

fsateler commented Nov 23, 2015

Pbuilder by default enters (on linux) a new network namespace and only brings up the loopback address there.

This can be disabled setting USENETWORK=yes in /etc/pbuilderrc

I'm wondering, given that the test is already running in a VM, in this case would it be better to disable this feature, so that more tests are exercised?

@mvollmer

This comment has been minimized.

Show comment
Hide comment
@mvollmer

mvollmer Nov 23, 2015

Member

[...] it's usually recommended to use dh-autoreconf, which will keep track of the autogenerated "stuff" and properly remove it again on clean.

Done, thanks for the tip!

Member

mvollmer commented Nov 23, 2015

[...] it's usually recommended to use dh-autoreconf, which will keep track of the autogenerated "stuff" and properly remove it again on clean.

Done, thanks for the tip!

@mvollmer

This comment has been minimized.

Show comment
Hide comment
@mvollmer

mvollmer Nov 23, 2015

Member

More action: https://mvo.fedorapeople.org/logs/test-0eb9d55c-debian-8/log.html

All integration tests fail, but at least they run! :-)

Member

mvollmer commented Nov 23, 2015

More action: https://mvo.fedorapeople.org/logs/test-0eb9d55c-debian-8/log.html

All integration tests fail, but at least they run! :-)

@stefwalter

This comment has been minimized.

Show comment
Hide comment
@stefwalter

stefwalter Nov 24, 2015

Contributor

there is a slight complication to that: What happens if we have to patch the non-bundled .js files from the tarball? Say for a security issue in angular, jquery, etc.

In that case each and every last devel dependency (all the nodejs npm deps) would be necessary during build to regenerate everything. In addition Cockpit would need to have code that pulls in external versions of jQuery and friends during build.

FWIW, it is not possible to pull in these dependencies at runtime. It is theoretically possible to pull them in at build time (with above caveats and contributing the above changes).

Contributor

stefwalter commented Nov 24, 2015

there is a slight complication to that: What happens if we have to patch the non-bundled .js files from the tarball? Say for a security issue in angular, jquery, etc.

In that case each and every last devel dependency (all the nodejs npm deps) would be necessary during build to regenerate everything. In addition Cockpit would need to have code that pulls in external versions of jQuery and friends during build.

FWIW, it is not possible to pull in these dependencies at runtime. It is theoretically possible to pull them in at build time (with above caveats and contributing the above changes).

@mvollmer mvollmer removed the blocked label Nov 30, 2015

@mvollmer

This comment has been minimized.

Show comment
Hide comment
@mvollmer

mvollmer Nov 30, 2015

Member

Intermediate status report:

  • This starts to look pretty good.
  • One big difference is the "wheel" vs "sudo" group thing. I still have to check carefully whetehr I got all instances. We need to translate those when synching users.
  • Debian has SELinux but it is disabled by default, it seems.
  • Debian passwd doesn't understand --stdin. We can use chpasswd instead.
  • Output of passwd -S differs. Passwd/libuser/shadow looks like a bit of a portability mess in general.
  • Small number of minor differences like /usr/bin/date vs /bin/date. Debian hasn't unified / and /usr.
  • newusers is missing the --crypto-method option, have to find an alternative
  • NetworkManager and FreeIPA/realmd tests are failing, haven't looked yet at all.
Member

mvollmer commented Nov 30, 2015

Intermediate status report:

  • This starts to look pretty good.
  • One big difference is the "wheel" vs "sudo" group thing. I still have to check carefully whetehr I got all instances. We need to translate those when synching users.
  • Debian has SELinux but it is disabled by default, it seems.
  • Debian passwd doesn't understand --stdin. We can use chpasswd instead.
  • Output of passwd -S differs. Passwd/libuser/shadow looks like a bit of a portability mess in general.
  • Small number of minor differences like /usr/bin/date vs /bin/date. Debian hasn't unified / and /usr.
  • newusers is missing the --crypto-method option, have to find an alternative
  • NetworkManager and FreeIPA/realmd tests are failing, haven't looked yet at all.
Show outdated Hide outdated tools/debian/copyright
@mbiebl

This comment has been minimized.

Show comment
Hide comment
@mbiebl

mbiebl Dec 1, 2015

Contributor

@dperpeet The debian/copyright file is indeed not complete. See my note in debian/TODO

  • Update/Review debian/copyright (e.g. for external resources)

Linking to external / online files is not allowed in the Debian policy. The debian/copyright file is supposed to be complete so it can be read offline.

Contributor

mbiebl commented Dec 1, 2015

@dperpeet The debian/copyright file is indeed not complete. See my note in debian/TODO

  • Update/Review debian/copyright (e.g. for external resources)

Linking to external / online files is not allowed in the Debian policy. The debian/copyright file is supposed to be complete so it can be read offline.

@mbiebl

This comment has been minimized.

Show comment
Hide comment
@dperpeet

This comment has been minimized.

Show comment
Hide comment
@dperpeet

dperpeet Dec 1, 2015

Member

Linking to external / online files is not allowed in the Debian policy. The debian/copyright file is supposed to be complete so it can be read offline.

Interesting, I didn't know that. Is it enough to say "Cockpit contributors"? Or does everyone have to be named?

Member

dperpeet commented Dec 1, 2015

Linking to external / online files is not allowed in the Debian policy. The debian/copyright file is supposed to be complete so it can be read offline.

Interesting, I didn't know that. Is it enough to say "Cockpit contributors"? Or does everyone have to be named?

@mbiebl

This comment has been minimized.

Show comment
Hide comment
@mbiebl

mbiebl Dec 1, 2015

Contributor

Good question. The opinions differ on that.
See the very recent discussion at https://lists.debian.org/debian-devel/2015/11/msg00235.html
Some Debian maintainers go as far as listing each copyright holder for each file.

I usually try to combine entries.
Say a project consists of parts that are under LGPL, MIT and GPL, then I usually add individual sections for each license.

See http://metadata.ftp-master.debian.org/changelogs//main/d/dbus/dbus_1.10.4-1_copyright as an example.

Contributor

mbiebl commented Dec 1, 2015

Good question. The opinions differ on that.
See the very recent discussion at https://lists.debian.org/debian-devel/2015/11/msg00235.html
Some Debian maintainers go as far as listing each copyright holder for each file.

I usually try to combine entries.
Say a project consists of parts that are under LGPL, MIT and GPL, then I usually add individual sections for each license.

See http://metadata.ftp-master.debian.org/changelogs//main/d/dbus/dbus_1.10.4-1_copyright as an example.

Show outdated Hide outdated tools/debian/control
Show outdated Hide outdated tools/debian/control
@@ -42,7 +54,8 @@ class TestLoopback(MachineCase):
b.wait_visible('#login-fatal')
self.assertIn("The server refused to authenticate 'admin' using password authentication, and no other supported authentication methods are available.", b.text('#login-fatal'));
self.allow_journal_messages("cockpit-polkit: couldn't respond to polkit daemon: GDBus\\.Error:org\\.freedesktop\\.PolicyKit1\\.Error\\.Failed: No session for cookie")
self.allow_journal_messages("cockpit-polkit: couldn't respond to polkit daemon: GDBus\\.Error:org\\.freedesktop\\.PolicyKit1\\.Error\\.Failed: No session for cookie",
".*server offered unsupported authentication methods: public-key.*")

This comment has been minimized.

@dperpeet

dperpeet Dec 2, 2015

Member

Does the last filter only apply to debian? We could add it to the allowed list conditionally then.

@dperpeet

dperpeet Dec 2, 2015

Member

Does the last filter only apply to debian? We could add it to the allowed list conditionally then.

This comment has been minimized.

@stefwalter

stefwalter Dec 2, 2015

Contributor

I think the message seems general enough. It could happen elsewhere.

@stefwalter

stefwalter Dec 2, 2015

Contributor

I think the message seems general enough. It could happen elsewhere.

This comment has been minimized.

@mvollmer

mvollmer Dec 2, 2015

Member

We should figure out a bit more what is going on here.

The message really means "server offered only unsupported methods", which isn't true for Debian: it has offered the password method, which we support, but we didn't try because cockpit-ws didn't have a password to use.

This code is inconsistent re password and public-key there: public-key is always considered supported (when compiled in), even if there is no key in cockpit-ws (or the agent) that could be used. On Fedora, where we do support public-key, we thus never see this message.

I'll propose this patch:

diff --git a/src/ws/cockpitsshtransport.c b/src/ws/cockpitsshtransport.c
index e0371c5..96e256b 100644
--- a/src/ws/cockpitsshtransport.c
+++ b/src/ws/cockpitsshtransport.c
@@ -578,6 +578,7 @@ cockpit_ssh_authenticate (CockpitSshData *data)
         }
       else if (!has_creds)
         {
+          methods_tried = methods_tried | method;
           result_string = "not-provided";
         }
       else

@petervo, what do you think?

@mvollmer

mvollmer Dec 2, 2015

Member

We should figure out a bit more what is going on here.

The message really means "server offered only unsupported methods", which isn't true for Debian: it has offered the password method, which we support, but we didn't try because cockpit-ws didn't have a password to use.

This code is inconsistent re password and public-key there: public-key is always considered supported (when compiled in), even if there is no key in cockpit-ws (or the agent) that could be used. On Fedora, where we do support public-key, we thus never see this message.

I'll propose this patch:

diff --git a/src/ws/cockpitsshtransport.c b/src/ws/cockpitsshtransport.c
index e0371c5..96e256b 100644
--- a/src/ws/cockpitsshtransport.c
+++ b/src/ws/cockpitsshtransport.c
@@ -578,6 +578,7 @@ cockpit_ssh_authenticate (CockpitSshData *data)
         }
       else if (!has_creds)
         {
+          methods_tried = methods_tried | method;
           result_string = "not-provided";
         }
       else

@petervo, what do you think?

This comment has been minimized.

@petervo

petervo Dec 2, 2015

Contributor

On 12/02/2015 05:29 AM, Marius Vollmer wrote:

In test/check-loopback

The message really means "server offered /only/ unsupported methods",
which isn't true for Debian: it has offered the password method, which
we support, but there we didn't try because cockpit-ws didn't have a
password to use.

I don't think that's true, the test disables password authentication, so
the server doesn't offer it. It only offered public-key which we don't
support yet on debian so the message seems right to me.

This code is inconsistent re password and public-key there: public-key
is always considered supported (when compiled in), even if there is no
key in cockpit-ws (or the agent) that could be used. On Fedora, where we
do support public-key, we thus never see this message.

It is true that there is an inconsistency between public-key and
password. not-supported should work correctly with public-key but we
don't check that we actually have a valid agent with keys so we never
respond with not-provided when there are none in the same way as we do
when no password is supplied. I'm not sure that's worth fixing though as
querying the agent here is not trivial but if everyone thinks that is
worth doing I can take shot at it.


Reply to this email directly or view it on GitHub
https://github.com/cockpit-project/cockpit/pull/3202/files#r46413264.

@petervo

petervo Dec 2, 2015

Contributor

On 12/02/2015 05:29 AM, Marius Vollmer wrote:

In test/check-loopback

The message really means "server offered /only/ unsupported methods",
which isn't true for Debian: it has offered the password method, which
we support, but there we didn't try because cockpit-ws didn't have a
password to use.

I don't think that's true, the test disables password authentication, so
the server doesn't offer it. It only offered public-key which we don't
support yet on debian so the message seems right to me.

This code is inconsistent re password and public-key there: public-key
is always considered supported (when compiled in), even if there is no
key in cockpit-ws (or the agent) that could be used. On Fedora, where we
do support public-key, we thus never see this message.

It is true that there is an inconsistency between public-key and
password. not-supported should work correctly with public-key but we
don't check that we actually have a valid agent with keys so we never
respond with not-provided when there are none in the same way as we do
when no password is supplied. I'm not sure that's worth fixing though as
querying the agent here is not trivial but if everyone thinks that is
worth doing I can take shot at it.


Reply to this email directly or view it on GitHub
https://github.com/cockpit-project/cockpit/pull/3202/files#r46413264.

This comment has been minimized.

@mvollmer

mvollmer Dec 3, 2015

Member

it has offered the password method,

I don't think that's true, the test disables password authentication,

You are right. This message does however also happen in other cases where password authentication has not been disabled but cockpit-ws doesn't have a password.

I guess I am just confused about what the problem codes authentication-not-supported, authentication-failed, and no-forwarding mean exactly.

Would it make sense to remove authentication-not-supported and no-forwarding completely (by returning authentication-failed instead) and just use the more explicit auth-methods-results member?

@mvollmer

mvollmer Dec 3, 2015

Member

it has offered the password method,

I don't think that's true, the test disables password authentication,

You are right. This message does however also happen in other cases where password authentication has not been disabled but cockpit-ws doesn't have a password.

I guess I am just confused about what the problem codes authentication-not-supported, authentication-failed, and no-forwarding mean exactly.

Would it make sense to remove authentication-not-supported and no-forwarding completely (by returning authentication-failed instead) and just use the more explicit auth-methods-results member?

This comment has been minimized.

@petervo

petervo Dec 3, 2015

Contributor

Would it make sense to remove authentication-not-supported and
no-forwarding completely (by returning authentication-failed instead)
and just use the more explicit auth-methods-results member?

I'm going to try to clean this up when I get around to #3269. We'll
remove no-forwarding and only return authentication-not-supported when
truly nothing is supported instead of not tried. login.html will need to
check auth-methods-results for displaying it's error. Sound Good?

@petervo

petervo Dec 3, 2015

Contributor

Would it make sense to remove authentication-not-supported and
no-forwarding completely (by returning authentication-failed instead)
and just use the more explicit auth-methods-results member?

I'm going to try to clean this up when I get around to #3269. We'll
remove no-forwarding and only return authentication-not-supported when
truly nothing is supported instead of not tried. login.html will need to
check auth-methods-results for displaying it's error. Sound Good?

Show outdated Hide outdated test/check-roles
Show outdated Hide outdated test/check-terminal
Show outdated Hide outdated test/testvm.py
Show outdated Hide outdated pkg/base1/cockpit.js
Show outdated Hide outdated test/check-terminal

mvollmer added some commits Nov 24, 2015

test: New get_admin_group function
To abstract away the difference between Fedora and Debian.
base1: More abstract superuser permission checks
Instead of a specific group, a Permissions object can now ask for
generic 'become superuser' capability.  This avoids hard coding
"wheel" all over the place.
users: Use chpasswd instead of passwd --stdin
The latter does not work on Debian.
test: Remove Fedora-isms from check-networking
- Pre-create new connection settings with nmcli instead of writing
  into a magical file.

- Don't assume that the main machine interface is managed by
  NetworkManager.
test: Use terminal instead of network in check-embed
The network module is not shipped in Debian.
test: Allow image specific known issues
By also looking into "./naughty-$IMAGE" if it exists.

@mvollmer mvollmer removed the needswork label Dec 21, 2015

@mvollmer mvollmer closed this in 25b0791 Dec 21, 2015

mvollmer added a commit that referenced this pull request Dec 21, 2015

test: Add "debian-unstable" to the list of automatically tested images.
Closes #3202
Reviewed-by: Stef Walter <stefw@redhat.com>
@mvollmer

This comment has been minimized.

Show comment
Hide comment
@mvollmer

mvollmer Jan 4, 2016

Member

Ok, summary:

  • Storage is disabled because there is no storaged in Debian yet.
  • Networking is disabled because the experience is not good enough.
  • Kubernetes is disabled because there is no kubernetes in Debian yet.
  • PCP is disabled because nobody has looked at it yet.

@mbiebl, @fsateler, this pull request has been merged into master, and we are now routinely testing every pull request against debian-unstable.

What could the next steps be?

Do you think we need to address some of the points above before proposing cockpit for inclusion in the Debian archive?

Member

mvollmer commented Jan 4, 2016

Ok, summary:

  • Storage is disabled because there is no storaged in Debian yet.
  • Networking is disabled because the experience is not good enough.
  • Kubernetes is disabled because there is no kubernetes in Debian yet.
  • PCP is disabled because nobody has looked at it yet.

@mbiebl, @fsateler, this pull request has been merged into master, and we are now routinely testing every pull request against debian-unstable.

What could the next steps be?

Do you think we need to address some of the points above before proposing cockpit for inclusion in the Debian archive?

@mvollmer mvollmer referenced this pull request Feb 15, 2016

Closed

fresh debian image #3727

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment