Skip to content

Set default values for each property of versions object#3268

Merged
DL6ER merged 2 commits into
developmentfrom
footer_null_alternative
Mar 7, 2025
Merged

Set default values for each property of versions object#3268
DL6ER merged 2 commits into
developmentfrom
footer_null_alternative

Conversation

@rdwebdesign

@rdwebdesign rdwebdesign commented Mar 2, 2025 •
edited
Loading

Copy link
Copy Markdown
Member

What does this PR aim to accomplish?

Fixes issues when versions file contains empty or null versions, to avoid an empty footer or invalid version descriptions.

If local.version is null, the old code completely hides the component version.

See:

How does this PR accomplish the above?

EDIT:

Now FTL will never send an empty string. If FTL can't find a valid string it will send null.

If local.version is null, the footer code will convert it to "N/A", to avoid empty footer or invalid versions.

By submitting this pull request, I confirm the following:

  1. I have read and understood the contributors guide, as well as this entire template. I understand which branch to base my commits and Pull Requests against.
  2. I have commented my proposed changes within the code and I have tested my changes.
  3. I am willing to help maintain this change if there are issues with it later.
  4. It is compatible with the EUPL 1.2 license
  5. I have squashed any insignificant commits. (git rebase)
  6. I have checked that another pull request for this purpose does not exist.
  7. I have considered, and confirmed that this submission will be valuable to others.
  8. I accept that this submission may not be used, and the pull request closed at the will of the maintainer.
  9. I give this submission freely, and claim no ownership to its content.
  • I have read the above and my PR is ready for review. Check this box to confirm

@rdwebdesign
Setting default values for each property of versions object
54e5b29 
- set Docker versions (local/remote) to `null` if they are empty strings, undefined or null;
- set other components versions to `"N/A"` if they are empty strings, undefined or null;
- set branches and hashes to `null` if they are empty strings, undefined or null;

Signed-off-by: RD WebDesign <github@rdwebdesign.com.br>
@rdwebdesign rdwebdesign mentioned this pull request Mar 2, 2025
Closed
1 task
@DL6ER

DL6ER commented Mar 2, 2025

Copy link
Copy Markdown
Member

This should be done in the API and the code here should be rewritten to not needing the immediate versions object at all

@rdwebdesign

rdwebdesign commented Mar 2, 2025

Copy link
Copy Markdown
Member Author

It's another possibility.

@rdwebdesign

rdwebdesign commented Mar 5, 2025

Copy link
Copy Markdown
Member Author

@DL6ER

Do you want to solve the issue in FTL or should I mark this PR as "ready for review"?

@DL6ER

DL6ER commented Mar 5, 2025

Copy link
Copy Markdown
Member

I already did this somewhere these days but seemingly forgot to push the branch. Will have a look for it after work.

@DL6ER DL6ER mentioned this pull request Mar 5, 2025
Merged
5 tasks
@DL6ER

DL6ER commented Mar 5, 2025

Copy link
Copy Markdown
Member

Is this really needed? Looking into the FTL source code, this exact usecase seems to be already there. Okay, no N/A for branch but there should never be items missing. If there are no corresponding keys in the version file, FTL will/should substitute null instead, see https://github.com/pi-hole/FTL/blob/b7eb53bf32ab76546db87c6db6d7085526788d67/src/api/info.c#L825-L864

I guess what is really needed here is a small fix of the parse_line() function skipping lines without values like

CORE_BRANCH=

This will effectively avoid an empty string being returned by the API and, hence, the automatic substitution with the nulls in the code references above. See PR pi-hole/FTL#2335

@yubiuser

yubiuser commented Mar 5, 2025

Copy link
Copy Markdown
Member

Even with pi-hole/FTL#2335 we need some small tweak to avoid the empty component to disappear

2025-03-05_20-25

@rdwebdesign

rdwebdesign commented Mar 5, 2025 •
edited
Loading

Copy link
Copy Markdown
Member Author

Even with pi-hole/FTL#2335 we need some small tweak to avoid the empty component to disappear

With the new FTL branch, only the .local.version values need a default value (|| "N/A") in this branch. Maybe FTL can send "N/A" instead of null, in cases like this.

EDIT:

If local.version is null the component won't be printed on the footer.
It doesn't really matter if remote.version is null or "N/A" (it will work in both cases).

Using the new FTL branch, the API is sending:

        "web":{
            "local":{
                "branch":null,
                "version":null,
                "hash":null
            },
            "remote":{
                "version":null,
                "hash":"54e5b294"
            }
        },

We can avoid this web PR is FTL sends something like this:

        "web":{
            "local":{
                "branch":null,
                "version":"N/A",
                "hash":null
            },
            "remote":{
                "version":"N/A",
                "hash":"54e5b294"
            }
        },

@rdwebdesign
Use "N/A" if local.version is null to avoid empty version
c3d09c9 
Signed-off-by: RD WebDesign <github@rdwebdesign.com.br>
@rdwebdesign rdwebdesign marked this pull request as ready for review March 7, 2025 04:47
yubiuser
yubiuser approved these changes Mar 7, 2025
View reviewed changes

@yubiuser yubiuser left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

2025-03-07_09-09

@DL6ER

DL6ER commented Mar 7, 2025

Copy link
Copy Markdown
Member

I am going back and forth about this but I think it's more natural (as in: computer-friendly) for the API to consistently reply with null and then having this PR to convert that to something more human-friendly.

@DL6ER DL6ER merged commit 27696fa into development Mar 7, 2025
@DL6ER DL6ER deleted the footer_null_alternative branch March 7, 2025 10:39
@PromoFaux PromoFaux mentioned this pull request Mar 29, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@yubiuser yubiuser yubiuser approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@rdwebdesign @DL6ER @yubiuser