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

Fix NuageVsp errors for build-master-slowbuild #1092

Merged
merged 1 commit into from
Nov 27, 2015
Merged

Conversation

nlivens
Copy link

@nlivens nlivens commented Nov 19, 2015

No description provided.

@nlivens
Copy link
Author

nlivens commented Nov 19, 2015

@DaanHoogland, this PR contains the fixes for the build-master-slowbuild

@DaanHoogland
Copy link
Contributor

thanks @nlivens lgtm but will run regression tests.

@fmaximus
Copy link
Contributor

I think we should use StringUtils.getUtf8Bytes() instead, as we are using StringUtils.newStringUtf8(Base64.decodeBase64(cmsUserPassBase64)) on the other side (NuageVspResource:192)

Otherwise it would fail if StringUtils.getPreferredCharset() is different from UTF-8

@DaanHoogland
Copy link
Contributor

@fmaximus preffered charset will be utf8 if it is available in the virtual machine (afaik always) otherwise it is the platform default

@DaanHoogland
Copy link
Contributor

1092.test_network.results.txt
1092.test_vpc.results.txt

Note that the two errors test destoying the system VMs. These timeout but the system VMs come back after the timeout.

Test destroy SSVM ... === TestName: test_09_destroy_ssvm | Status : EXCEPTION ===
ERROR
Test destroy CPVM ... === TestName: test_10_destroy_cpvm | Status : EXCEPTION ===
ERROR

LGTM

@nlivens
Copy link
Author

nlivens commented Nov 20, 2015

@DaanHoogland, @fmaximus was right about the encoding. I rewrote that part so we're always using the same encoding :)

@DaanHoogland
Copy link
Contributor

@nlivens @fmaximus I have to ask you to reconsider. if getPreffferedCharset() does not return UTF8 it is because it is not supported on the platform. how do you handle that situation?

@nlivens
Copy link
Author

nlivens commented Nov 20, 2015

@DaanHoogland, how about the following piece of code then? In this case we are using the getPrefferedCharset()

    public static String encodePassword(String originalPassword) {
        byte[] passwordBytes = originalPassword.getBytes(StringUtils.getPreferredCharset());
        byte[] encodedPasswordBytes = Base64.encodeBase64(passwordBytes);
        return new String(encodedPasswordBytes, StringUtils.getPreferredCharset());
    }

    public static String decodePassword(String encodedPassword) {
        byte[] encodedPasswordBytes = encodedPassword.getBytes(StringUtils.getPreferredCharset());
        byte[] passwordBytes = Base64.decodeBase64(encodedPasswordBytes);
        return new String(passwordBytes, StringUtils.getPreferredCharset());
    }

@DaanHoogland
Copy link
Contributor

@nlivens looks good, but I have some questions still

  1. So why wouldn't you encorporate this in your getBytesUtf8() and newStringUtf8() methods?
  2. How will the plugin behave when utf8 is not supported (not sure if it will ever occur but let's say running on the level frame OS)?

@nlivens
Copy link
Author

nlivens commented Nov 20, 2015

@DaanHoogland

  1. getBytesUtf8() and newStringUtf8() would no longer be used.
  2. With this new code, we won't any longer be only dependent on UTF-8, but rather on the Charset returned by StringUtils.getPreferredCharset().

You can have a look at the newly pushed code :)

@DaanHoogland
Copy link
Contributor

@nlivens .1 : my bad .2 remains but is not specific to this patch and even this plugin. LGTM I asked @remibergsma on slack to have a look as well.

@remibergsma
Copy link
Contributor

LGTM, all integration tests pass.

screen shot 2015-11-22 at 20 24 54 pm

@rafaelweingartner
Copy link
Member

The code seems ok, but what about a PR description and test cases for those encode and decode methods that were created?

@nlivens
Copy link
Author

nlivens commented Nov 25, 2015

@rafaelweingartner, I've added a test for those methods

@rafaelweingartner
Copy link
Member

@nvlivens,
I would suggest you to write two test cases instead of one.
One test case to test the encode method and another to test the decode method.

@nlivens
Copy link
Author

nlivens commented Nov 25, 2015

@rafaelweingartner, implemented the test cases as suggested

@rafaelweingartner
Copy link
Member

The code LGTM now

@nlivens
Copy link
Author

nlivens commented Nov 26, 2015

@DaanHoogland, @remibergsma, can this be merged in?

@asfgit asfgit merged commit 453333c into apache:master Nov 27, 2015
asfgit pushed a commit that referenced this pull request Nov 27, 2015
Fix NuageVsp errors for build-master-slowbuild

* pr/1092:
  Fix NuageVsp errors for build-master-slowbuild

Signed-off-by: Remi Bergsma <github@remi.nl>
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

6 participants