Log everything to stderr #93
Conversation
dcermak
left a comment
dcermak
left a comment
There was a problem hiding this comment.
What happens now if the container-suseconnect plugin fails to obtain the repo for whatever reason? Will zypper ref fail completely or will the SLE_BCI repo still be usable on BCI?
|
@dcermak according to my tests, if
|
|
Ah, but please add an entry to the |
|
@dcermak the changelog is outdated or forgotten, the version there is 2.3, while the version released is 2.5. I have so many changes in mind, plus a few changes in behavior like this PR, that I'm considering the next version to be 3.0, not sure if 2.6 would fit such changes. GitHub releases have the entire changelog and OBS can fetch that, perhaps not worth having an extra file. WDYT? |
I'm fine with a new major version, that's warranted given the bigger changes.
Github releases will by default only list the commit log, which is not suitable for end user consumption. I think a user consumable changelog still provides value. |
alexandrevicenzi
force-pushed
the
zypper-log-error
branch
from
4b27a7e to
17f61e0
Compare
|
@dcermak I'll try to keep the changelog, but I'll update it in a future PR with all changes that are going in the next release. |
alexandrevicenzi
force-pushed
the
zypper-log-error
branch
2 times, most recently
from
920e907 to
6c4951c
Compare
| } | ||
| func TestIsValidLogPathFromRegularFile(t *testing.T) { | ||
| tempFile, err := os.CreateTemp("", "prefix") | ||
| assert.Nil(t, err) |
There was a problem hiding this comment.
iirc assert is for tests, require is for prerequisites
| assert.Nil(t, err) | |
| require.Nil(t, err) |
There was a problem hiding this comment.
require is just a wrapper around assert that calls t.FailNow. I don't see any value in this change to be honest.
Am I missing something?
There was a problem hiding this comment.
If the creation of the temporary directory fails, then there's really no value in continuing to execute the test, as it is already clear that it will fail too. That was my rationale, why require should be used here
There was a problem hiding this comment.
I'll make a few tests, but for now I'll leave it like this. My idea is to rework other tests to use this library as it makes it easier to test and clearer to understand.
| testLogger(t, "suse.log", "suse.log", true) | ||
| func TestIsValidLogPathFromValidDir(t *testing.T) { | ||
| dir, err := os.MkdirTemp("", "") | ||
| assert.Nil(t, err) |
There was a problem hiding this comment.
| assert.Nil(t, err) | |
| require.Nil(t, err) |
|
|
||
| func TestIsValidLogPathFromMissingDir(t *testing.T) { | ||
| dir, err := os.MkdirTemp("", "") | ||
| assert.Nil(t, err) |
There was a problem hiding this comment.
| assert.Nil(t, err) | |
| require.Nil(t, err) |
alexandrevicenzi
force-pushed
the
zypper-log-error
branch
from
6c4951c to
b9b66e9
Compare
Always write logs to both a file and Stderr as it makes easier to debug issues. Signed-off-by: Alexandre Vicenzi <[email protected]>
There's no reason to hide such an error from the user. Users must be aware that a container has no valid credentials. Zypper will forward any errors encountered in plugins, allowing users to be fully aware why repos and packages are not showing up. Signed-off-by: Alexandre Vicenzi <[email protected]>
alexandrevicenzi
force-pushed
the
zypper-log-error
branch
from
b9b66e9 to
5569598
Compare
dcermak
left a comment
dcermak
left a comment
There was a problem hiding this comment.
Looks much better now, thanks!
This reverts SUSE#93 partially because it causes zyppers to log errors every time it fails to locate credentials files. Failures are still logged to a file, but not propagated to zypper.
This reverts SUSE#93 partially because it causes zyppers to log errors every time it fails to locate credentials files. Failures are still logged to a file, but not propagated to zypper.
2 participants
Zypper is capable of propagating error messages written to stderr if the application exits with a non-zero code.
This fixes #91.