Fixes #37900 - Allow syncing templates through HTTP proxy #191
Conversation
adamlazik1
force-pushed
the
sync-templates
branch
2 times, most recently
from
757bb35
to
fa2aab3
Compare
|
I've just tested the sweet paths and can confirm API part works. |
|
OK, even WebUI part seems to work. Is my testing wrong or is that expected? My last info was that this should only work in API. |
|
@lhellebr What I currently don't like about the web ui is that the the HTTP proxy selection box appears even when you have global proxy or no proxy selected in the proxy policy box. This could potentially confuse the user. I am currently trying to fix that. The behavior where the selection box appears only when the proxy policy is "selected" would be also consistent with for example the one from katello - on the Content -> Products -> Repositories -> New repository page, where they handle HTTP proxies the same way. Unfortunately the front end there uses different technology so I cannot copy the code. I am very close to the solution, though. |
adamlazik1
force-pushed
the
sync-templates
branch
2 times, most recently
from
8f60cd0
to
65c6644
Compare
|
Alright, now the proxy id field is displayed only when it's supposed to but I've broken the ui procedure as proxy params are currently not sent to the controller. I also need to figure out how to make translations work on the new fields. On the plus side, username and password for a proxy is now used correctly. I still need to setup config for SSL cert. I'll have to continue on this next week. |
adamlazik1
force-pushed
the
sync-templates
branch
6 times, most recently
from
ce66adb
to
ce1a701
Compare
|
Right now everything works if there is at least one proxy to choose but things break in the UI if there is no proxy available. I will have to fix that next week. |
adamlazik1
force-pushed
the
sync-templates
branch
from
ce1a701
to
e1f3c95
Compare
webpack/components/NewTemplateSync/components/NewTemplateSyncForm/NewTemplateSyncFormHelpers.js
Show resolved
Hide resolved
adamlazik1
force-pushed
the
sync-templates
branch
from
7352af6
to
11712aa
Compare
adamlazik1
force-pushed
the
sync-templates
branch
from
11712aa
to
29dc629
Compare
|
Fixed the last issue that I found and simplified the code a bit. From the functionality side this should be fully functional. I still need to add tests. |
adamlazik1
force-pushed
the
sync-templates
branch
3 times, most recently
from
8397ca2
to
bcbc646
Compare
|
|
In WebUI, when syncing with default form values, just specifying branch to The templates still seem to be applied. The messages about template being locked are shown in stream without this PR as well. |
I tried this on the master branch and got the same result, it seems that it is unrelated to this PR. I will try to find the cause. |
adamlazik1
force-pushed
the
sync-templates
branch
4 times, most recently
from
1e57bc4
to
6237b86
Compare
|
The unit tests here should hopefully be enough. I don't know if I should create any API controller tests because I can't simulate an HTTP proxy and can't think about testing anything else that would make sense. If someone has any ideas, I can add more tests, but until then I consider the added tests sufficient. |
|
adamlazik1
force-pushed
the
sync-templates
branch
2 times, most recently
from
37ee56c
to
8d0d041
Compare
|
@asteflova here are the new descriptions on settings and sync page, respectively, based on what we discussed. |
adamlazik1
force-pushed
the
sync-templates
branch
from
8d0d041
to
69823ba
Compare
|
2 More issues (with a few days old version, maybe you fixed it already, let me know) When I select In Settings -> Template sync, I can set policy but there is no field to set the chosen proxy if I choose |
This should be fixed in the current version, the submit button is not clickable if there is no proxy to select.
This is intentional, since HTTP proxies are scoped by taxonomies, whereas settings are global. Adding HTTP proxy to settings would mean it would be visible outside of its assigned organizations and locations, which I don't think is correct. |
|
Understood! |
adamlazik1
force-pushed
the
sync-templates
branch
from
69823ba
to
338a62b
Compare
There was a problem hiding this comment.
The ruby part looks sensible, @MariaAga could you please take a look at the frontend part?
| const descriptionEdits = { | ||
| '? ': '?<br>', | ||
| 'Custom HTTP proxy': '<b>Custom HTTP proxy</b>', | ||
| ' when syncing templates': '', | ||
| }; | ||
| proxyPolicySetting = proxyPolicySetting.set( | ||
| 'description', | ||
| `${proxyPolicySetting.description.replace( | ||
| /\b(\? |Custom HTTP proxy| when syncing templates)\b/g, | ||
| match => descriptionEdits[match] | ||
| )}` | ||
| ); |
There was a problem hiding this comment.
This causes the description not to be translated.
N_('Should an HTTP proxy be used for template sync? If you select Custom HTTP proxy, you will be prompted to select one when syncing templates.') - this marks it to be translated in the future, for example in the js code with __(proxyPolicySetting.description).
Maybe it will be better to use the
in lib/foreman_templates/engine.rb but I'm not sure if the description is used anywhere else
There was a problem hiding this comment.
Thank you for pointing that out.
I removed the description edits, but I have follow-up questions.
- In
webpack/components/NewTemplateSync/components/SyncSettingFields.js:22:
const modifyDescription = (setting, type) => {
if (setting.description) {
let split = setting.description.split('. ');
if (setting.name === 'repo' && type !== 'export') {
split = split.slice(0, split.length - 1);
}
split = split.join('.<br>');
return setting.set('description', split);
}
return setting;
};
This piece of code was present here before and it modifies the description in the same way as me. Wouldn't that cause the description to also not be translated?
2. What about strings that are marked for translation in the UI but have no counterpart in the backend, like in webpack/components/NewTemplateSync/components/NewTemplateSyncForm/NewTemplateSyncFormHelpers.js:23?
export const syncFormSchema = (syncType, settingsObj, validationData) => {
const schema = (settingsObj[syncType].asMutable() || []).reduce(
(memo, setting) => {
if (setting.name === 'repo') {
return {
...memo,
repo: Yup.string()
.test(
'repo-format',
`${__(
'Invalid repo format, must start with one of: '
)}${validationData.repo.join(', ')}`,
...
There was a problem hiding this comment.
- yes, I didnt check the code before, but it looks like modifyDescription will not be translated.
- We can add new strings in the UI, that mark for translated is used so we can send variables to be translated:
translate_later = N('string')and then__(translate_later)
adamlazik1
force-pushed
the
sync-templates
branch
from
338a62b
to
ec10c78
Compare
| http_proxy_policy: Yup.mixed().test( | ||
| 'http-proxy-available', | ||
| __( | ||
| 'No HTTP proxies available. Please select a different policy or verify that you have selected the correct organization and location.' | ||
| ), |
There was a problem hiding this comment.
This is only visible after clicking "No HTTP proxies available" and also blocks the user form changing from import to export. I think it might be better to not show the option for a custom http proxy is the user doesnt have any (or, more complicated I think, to show the warning and only block submit)
No description provided.