Skip to content

Comments

Fix 5478 move Swift settings to domain.xml for better governability, no disk access of properties file, leaner class#5680

Merged
kcondon merged 12 commits intoIQSS:developfrom
carlosmcgregor:develop
Apr 11, 2019
Merged

Fix 5478 move Swift settings to domain.xml for better governability, no disk access of properties file, leaner class#5680
kcondon merged 12 commits intoIQSS:developfrom
carlosmcgregor:develop

Conversation

@carlosmcgregor
Copy link
Contributor

@carlosmcgregor carlosmcgregor commented Mar 22, 2019

Related Issues

Pull Request Checklist

Changes to Documentation

The following commands are now needed to configure Swift:

asadmin create-jvm-options "\-Ddataverse.file.swift-default-endpoint=endpoint1"
asadmin create-jvm-options "\-Ddataverse.file.swift-auth-type.endpoint1=$AUTH_TYPE"
asadmin create-jvm-options "\-Ddataverse.file.swift-auth-url.endpoint1=$AUTH_URL"
asadmin create-jvm-options "\-Ddataverse.file.swift-tenant.endpoint1=$TENANT"
asadmin create-jvm-options "\-Ddataverse.file.swift-username.endpoint1=$USERNAME"
asadmin create-jvm-options "\-Ddataverse.file.swift-password.endpoint1='${ALIAS=$PASSWORD}'"
asadmin create-jvm-options "\-Ddataverse.file.swift-endpoint.endpoint1=$ENDPOINT"
asadmin create-jvm-options "\-Ddataverse.file.swift-hash-key.endpoint1=$HASH_KEY"

It makes use of the create-password-alias approach that @pdurbin pointed out (thank you!). Otherwise, all settings remain the same as with swift.properties. Some additional settings that were hardcoded (i.e. separator and temporary URL expiry time) were moved to the settings. These are:

dataverse.files.swift-folder-path-separator
dataverse.files.swift-temporary-url-expiry-time

Please let me know if you have further questions regarding the changes that were made!

Copy link
Member

@pdurbin pdurbin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The docs at a future version of http://guides.dataverse.org/en/4.11/installation/config.html#swift-storage need to change, right?

That page can be found at doc/sphinx-guides/source/installation/config.rst if you could please edit it.

@carlosmcgregor
Copy link
Contributor Author

@pdurbin, I'm on it!

@coveralls
Copy link

coveralls commented Mar 22, 2019

Coverage Status

Coverage increased (+0.005%) to 18.066% when pulling e0ab352 on carlosmcgregor:develop into fe4dcfc on IQSS:develop.

@pdurbin pdurbin reopened this Mar 22, 2019
@poikilotherm
Copy link
Contributor

poikilotherm commented Mar 23, 2019

I am not sure if it's a good idea to move this to domain XML. For future platforms like Payara or Glassfish 5, the rising usage of configuration frameworks and more, this might be a long-term decision which is not easy to revert. After re-reading specs: you can actually follow that road - ConfigAPI is fine with this. But please read on, below is important.

At least pretty please avoid - and _ in the config names. - is an invalid char for environment variables, _ is used for substitution. See gdcc/dataverse-kubernetes#11 Best option is to use a "." for namespacing as used elsewhere plus camel / pascal case.

…entation

Add full create-password-alias instructions in documentation
Add missing swiftEndPoint
./asadmin $ASADMIN_OPTS create-jvm-options "\-Ddataverse.file.swift-username.endpoint1=your-username"
./asadmin $ASADMIN_OPTS create-jvm-options "\-Ddataverse.file.swift-password.endpoint1='${ALIAS=your-password}'"
./asadmin $ASADMIN_OPTS create-jvm-options "\-Ddataverse.file.swift-endpoint.endpoint1=your-swift-endpoint"
./asadmin $ASADMIN_OPTS create-jvm-options "\-Ddataverse.file.swift.defaultEndpoint=endpoint1"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some typos here. In the docs you use .file., but in the code .files. is used.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@poikilotherm good catch

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for noticing that! Fixed.

@carlosmcgregor
Copy link
Contributor Author

I read http://irclog.iq.harvard.edu/dataverse/2019-03-25#i_88874 and I would like to add one more setting that has -:

dataverse.files.storage-driver-id

@carlosmcgregor
Copy link
Contributor Author

Sorry! Closed by accident :/

@pdurbin
Copy link
Member

pdurbin commented Mar 26, 2019

I would like to add one more setting

@carlosmcgregor no problem, I pulled this out of code review until you're done. Please let us know when we should take another look. Thanks! Also, you're welcome to join us in IRC if you'd like. 😄

@carlosmcgregor
Copy link
Contributor Author

Sorry, @pdurbin ! I should have added that dataverse.files.storage-driver-id is out of the scope of this pull. Just mentioned it so that it can be added to a list of Dataverse JVM options that need to be changed :)

@pdurbin
Copy link
Member

pdurbin commented Mar 26, 2019

@carlosmcgregor oh! I understand now. It looks like @poikilotherm is handling some of those "dash" JVM options over at https://github.com/IQSS/dataverse-kubernetes/blob/655b92d60b1a09e109154c9c237525827dbf4dff/dataverse-k8s/bin/init_1_configure.sh#L94

I'll move this issue and pull request back to code review. Thanks for explaining the scope.

@poikilotherm
Copy link
Contributor

@carlosmcgregor Feel free to leave a comment on gdcc/dataverse-kubernetes#11 :-)

@carlosmcgregor
Copy link
Contributor Author

Thank you @pdurbin and @poikilotherm !

@kcondon kcondon self-assigned this Apr 9, 2019
@kcondon
Copy link
Contributor

kcondon commented Apr 10, 2019

@carlosmcgregor Hi, would you mind refreshing this branch from /develop? We've since released v4.12 and your branch shows 4.11. This is causing a build issue. Once this is adjusted I will test and merge. Thanks! Kevin

@carlosmcgregor
Copy link
Contributor Author

Done, @kcondon ! Thank you :)

@kcondon kcondon merged commit a371605 into IQSS:develop Apr 11, 2019
@pdurbin pdurbin added this to the 4.13 milestone Apr 22, 2019
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.

5 participants