Fixes #36982 - display short hostname on REX job pages #859

nofaralfasi · 2023-12-14T15:01:42Z

If the display_fqdn_for_hosts setting is set to false, show the short hostname on REX job pages.

Requires:

Fixes #36160 - add display_name field to Host GraphQL type foreman#9968

nofaralfasi · 2023-12-17T14:59:04Z

@MariaAga Any idea why the tests are failing?

MariaAga · 2023-12-18T09:53:08Z

TypeError: (0 , _ForemanContext.useForemanSettings) is not a function
It needs to have a mock in the __mocks__ folder

nofaralfasi · 2023-12-18T10:28:05Z

TypeError: (0 , _ForemanContext.useForemanSettings) is not a function It needs to have a mock in the __mocks__ folder

Would adding this line here resolve the issue?
export const useForemanSettings = () => {};

Or is there a way I can test it locally? I couldn't find how.

MariaAga · 2023-12-18T10:29:13Z

Yeah it should fix it. You can run npm install npm test in the plugin folder to test

ShimShtein

The changes to the ruby code seem quite OK, but the JS doesn't have to know about this setting. It should just use display_name where it is possible.

webpack/JobWizard/steps/HostsAndInputs/HostPreviewModal.js

webpack/JobWizard/steps/ReviewDetails/index.js

webpack/JobWizard/steps/form/SearchSelect.js

ShimShtein · 2023-12-24T15:34:18Z

webpack/JobWizard/steps/HostsAndInputs/SelectedChips.js

@@ -31,7 +33,7 @@ const SelectedChip = ({ selected, setSelected, categoryName }) => {
            onClick={() => deleteItem(id)}
            closeBtnAriaLabel={`Remove ${name}`}
          >
-            {name}
+            {displayFqdnForHosts ? name : name.split('.')[0]}


selected variable is a set of hosts returned by the API, they all have display_name property, you can use it here, as long as you change the map statement:
instead of:

selected.map(({ name, id }, index) => (...)

You can add the display_name:

selected.map(({ name, display_name, id }, index) => (...)

I think we are using here gql for the query, which means that we don't have access to the display_name property.

Looks like we are missing display_name from GQL definition in foreman. Let's add it first.

theforeman/foreman#9968 added to the description

webpack/JobWizard/autofill.js

ekohl

If this still depends on theforeman/foreman#9968, can you raise the minimum required Foreman version so we don't accidentally cherry pick it?

ekohl · 2024-01-03T11:24:50Z

webpack/JobWizard/steps/HostsAndInputs/HostPreviewModal.js

            <Button
              component="a"
-              href={foremanUrl(`/hosts/${host}`)}
+              href={foremanUrl(`/hosts/${host.name}`)}


I'd be tempted to use host IDs in URLs, not names to avoid breaking when you rename hosts. I know Foreman is inconsistent about that. This is why I think we should avoid constructing URLs all over the place and instead rely on helpers. foremanUrl is really an anti-pattern.

If the UI relies on it so much, we should do what GH did and include html_url in the API response.

ekohl · 2024-01-03T11:26:36Z

webpack/JobWizard/steps/ReviewDetails/index.js

@@ -84,7 +84,7 @@ const ReviewDetails = ({
      return __('No Target Hosts');
    }
    if (hosts.length === 1 || hosts.length === 2) {


Unrelated, but I'd have written < 3 and probably made 3 some constant.

app/helpers/remote_execution_helper.rb

stejskalleos

🍏 LGTM

Did a quick test with enabled and disabled setting, with the new UI job wizard,
and worked fine.
Setting is applied everywhere and the job execution went well.

@adamruzicka @MariaAga I'll leave the final ACK on you

adamruzicka · 2024-01-15T10:09:39Z

Could you please rebase? It should make packit turn green

ShimShtein · 2024-01-15T10:24:17Z

webpack/JobWizard/steps/HostsAndInputs/HostPreviewModal.js

            <Button
              component="a"
-              href={foremanUrl(`/hosts/${host}`)}
+              href={foremanUrl(`/new/hosts/${host.name}`)}


Shouldn't it be dependent on a switch?
@Ron-Lavi?

You are right, we should check user's settings here. I'll remove it and open a separate PR for that.

I saw the same behaviour across the app so I think it is OK..

Probably we can try to load the correct host path to the React Context Settings and use that helper across the app ( but that's for another PR ). :)

adamruzicka · 2024-01-17T10:41:20Z

Could you also deal with it here https://github.com/theforeman/foreman_remote_execution/blob/master/app/lib/actions/remote_execution/run_host_job.rb#L104 ?

nofaralfasi · 2024-01-18T16:10:22Z

Could you also deal with it here https://github.com/theforeman/foreman_remote_execution/blob/master/app/lib/actions/remote_execution/run_host_job.rb#L104 ?

Is there a reason for the inconsistency in saving the action attribute of the ForemanTasks model? Sometimes it stores the full hostname, and other times it uses the short hostname. Any idea why?

adamruzicka · 2024-01-18T16:19:39Z

Could you give an example?

nofaralfasi · 2024-01-18T16:29:11Z

ForemanTasks

When I set the Display FQDN for hosts setting to true, and I execute any remote job on multiple hosts, some of them have the action field containing the short hostname, while others have the full hostname.

Update:
When I debug it, the update query contains the correct hostname, but for some reason that changes afterward and I can't figure out where.

adamruzicka · 2024-01-19T11:48:14Z

I don't know in what environment you hit that, but it sounds like that either not all processes picked up your code changes or that not all processes picked up the setting change. Either way, I'm failing to reproduce it on a fresh nightly machine

nofaralfasi · 2024-01-22T10:59:26Z

I don't know in what environment you hit that, but it sounds like that either not all processes picked up your code changes or that not all processes picked up the setting change. Either way, I'm failing to reproduce it on a fresh nightly machine

Thanks. Restarting my local environment seems to have solved the issue for me. Everything works as expected now.

nofaralfasi · 2024-01-24T13:35:00Z

Is there anything else required before we proceed with the merge?

adamruzicka

On a relevant side note, what does RunHostJob#host_name (a thing usable in webhooks) end up being? Is it the short name or the fqdn?

app/lib/actions/remote_execution/run_host_job.rb

nofaralfasi · 2024-01-31T15:57:48Z

On a relevant side note, what does RunHostJob#host_name (a thing usable in webhooks) end up being? Is it the short name or the fqdn?

I think this shouldn't be effected at all, however I didn't test it. hostname should stay the fqdn.

If the `display_fqdn_for_hosts` setting is set to false, show the short hostname on REX job pages.

adamruzicka

Looks good

adamruzicka · 2024-02-20T09:45:54Z

Thank you @nofaralfasi !

pr-processor bot added the Not yet reviewed label Dec 14, 2023

nofaralfasi force-pushed the display_fqdn_for_hosts branch from 96812fe to acd3e18 Compare December 14, 2023 15:18

nofaralfasi force-pushed the display_fqdn_for_hosts branch 2 times, most recently from a0252cd to 68405c3 Compare December 20, 2023 15:30

ShimShtein suggested changes Dec 24, 2023

View reviewed changes

pr-processor bot added Waiting on contributor and removed Not yet reviewed labels Dec 24, 2023

nofaralfasi force-pushed the display_fqdn_for_hosts branch from 68405c3 to 3fe4eb7 Compare December 26, 2023 11:21

pr-processor bot added the Needs re-review label Dec 26, 2023

nofaralfasi force-pushed the display_fqdn_for_hosts branch 3 times, most recently from c4dad98 to a8b8619 Compare December 27, 2023 13:42

nofaralfasi marked this pull request as draft December 27, 2023 14:11

nofaralfasi force-pushed the display_fqdn_for_hosts branch from a8b8619 to 33ff5f6 Compare December 27, 2023 14:43

nofaralfasi marked this pull request as ready for review December 27, 2023 14:44

nofaralfasi force-pushed the display_fqdn_for_hosts branch from 33ff5f6 to cdf48ef Compare January 1, 2024 16:56

pr-processor bot removed the Waiting on contributor label Jan 1, 2024

nofaralfasi commented Jan 1, 2024

View reviewed changes

webpack/JobWizard/autofill.js Show resolved Hide resolved

ekohl reviewed Jan 3, 2024

View reviewed changes

nofaralfasi marked this pull request as draft January 3, 2024 11:28

nofaralfasi changed the title ~~Fixes #36982 - display short hostname on REX job pages~~ [WIP] Fixes #36982 - display short hostname on REX job pages Jan 3, 2024

nofaralfasi force-pushed the display_fqdn_for_hosts branch 2 times, most recently from b93143b to d8f5135 Compare January 7, 2024 09:29

nofaralfasi changed the title ~~[WIP] Fixes #36982 - display short hostname on REX job pages~~ Fixes #36982 - display short hostname on REX job pages Jan 7, 2024

nofaralfasi marked this pull request as ready for review January 7, 2024 09:30

stejskalleos reviewed Jan 9, 2024

View reviewed changes

app/helpers/remote_execution_helper.rb Show resolved Hide resolved

nofaralfasi force-pushed the display_fqdn_for_hosts branch 2 times, most recently from 5b87230 to 6a67c5d Compare January 9, 2024 10:47

nofaralfasi requested a review from stejskalleos January 10, 2024 16:26

stejskalleos approved these changes Jan 11, 2024

View reviewed changes

pr-processor bot removed the Needs re-review label Jan 11, 2024

nofaralfasi force-pushed the display_fqdn_for_hosts branch from 6a67c5d to f109215 Compare January 15, 2024 10:22

ShimShtein reviewed Jan 15, 2024

View reviewed changes

nofaralfasi force-pushed the display_fqdn_for_hosts branch from f109215 to 3240802 Compare January 18, 2024 16:07

nofaralfasi force-pushed the display_fqdn_for_hosts branch from 3240802 to b7ec528 Compare January 18, 2024 16:48

nofaralfasi force-pushed the display_fqdn_for_hosts branch from b7ec528 to b93fc23 Compare January 25, 2024 13:47

adamruzicka reviewed Jan 30, 2024

View reviewed changes

app/lib/actions/remote_execution/run_host_job.rb Outdated Show resolved Hide resolved

Fixes #36982 - display short hostname on REX job pages

dfaad4e

If the `display_fqdn_for_hosts` setting is set to false, show the short hostname on REX job pages.

nofaralfasi force-pushed the display_fqdn_for_hosts branch from b93fc23 to dfaad4e Compare February 19, 2024 16:55

adamruzicka approved these changes Feb 20, 2024

View reviewed changes

adamruzicka merged commit 50a621e into theforeman:master Feb 20, 2024
13 checks passed

nofaralfasi mentioned this pull request Jun 24, 2024

Fixes #37584 - Host details - Fix append domain setting theforeman/foreman#10217

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #36982 - display short hostname on REX job pages #859

Fixes #36982 - display short hostname on REX job pages #859

nofaralfasi commented Dec 14, 2023 •

edited

Loading

nofaralfasi commented Dec 17, 2023

MariaAga commented Dec 18, 2023

nofaralfasi commented Dec 18, 2023

MariaAga commented Dec 18, 2023 •

edited

Loading

ShimShtein left a comment

ShimShtein Dec 24, 2023

nofaralfasi Dec 25, 2023

ShimShtein Dec 27, 2023

nofaralfasi Dec 27, 2023

ekohl left a comment

ekohl Jan 3, 2024

ekohl Jan 3, 2024

stejskalleos left a comment

adamruzicka commented Jan 15, 2024

ShimShtein Jan 15, 2024

nofaralfasi Jan 25, 2024

Ron-Lavi Jan 25, 2024

adamruzicka commented Jan 17, 2024

nofaralfasi commented Jan 18, 2024

adamruzicka commented Jan 18, 2024

nofaralfasi commented Jan 18, 2024 •

edited

Loading

adamruzicka commented Jan 19, 2024

nofaralfasi commented Jan 22, 2024

nofaralfasi commented Jan 24, 2024

adamruzicka left a comment

nofaralfasi commented Jan 31, 2024

adamruzicka left a comment

adamruzicka commented Feb 20, 2024

Fixes #36982 - display short hostname on REX job pages #859

Fixes #36982 - display short hostname on REX job pages #859

Conversation

nofaralfasi commented Dec 14, 2023 • edited Loading

nofaralfasi commented Dec 17, 2023

MariaAga commented Dec 18, 2023

nofaralfasi commented Dec 18, 2023

MariaAga commented Dec 18, 2023 • edited Loading

ShimShtein left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ekohl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stejskalleos left a comment

Choose a reason for hiding this comment

adamruzicka commented Jan 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamruzicka commented Jan 17, 2024

nofaralfasi commented Jan 18, 2024

adamruzicka commented Jan 18, 2024

nofaralfasi commented Jan 18, 2024 • edited Loading

adamruzicka commented Jan 19, 2024

nofaralfasi commented Jan 22, 2024

nofaralfasi commented Jan 24, 2024

adamruzicka left a comment

Choose a reason for hiding this comment

nofaralfasi commented Jan 31, 2024

adamruzicka left a comment

Choose a reason for hiding this comment

adamruzicka commented Feb 20, 2024

nofaralfasi commented Dec 14, 2023 •

edited

Loading

MariaAga commented Dec 18, 2023 •

edited

Loading

nofaralfasi commented Jan 18, 2024 •

edited

Loading