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

Add a workaround around non-ascii and non-standard FQDNs #790

Merged
merged 1 commit into from
Aug 23, 2022

Conversation

matejmatuska
Copy link
Member

@matejmatuska matejmatuska commented Aug 3, 2022

Add a wrapper around socket.getfqdn() which returns invalid.hostname
in cases where hostname, or rather FQDN contains non-ascii and
non-standard characters.

This works around the issue that non-ascii characters can't be inserted
into leapp database.

For the standard for hostnames see hostname(7). The current
implementation doesn't check for length of hostname nor it's individual
segments.

Jira ref: OAMG-7102

@github-actions
Copy link

github-actions bot commented Aug 3, 2022

Thank you for contributing to the Leapp project!

Please note that every PR needs to comply with the Leapp Guidelines and must pass all tests in order to be mergable.
If you want to request a review or rebuild a package in copr, you can use following commands as a comment:

  • review please to notify leapp developers of review request
  • /packit copr-build to submit a public copr build using packit

To launch regression testing public members of oamg organization can leave the following comment:

  • /rerun to schedule basic regression tests using this pr build and leapp-repository*master* as artifacts
  • /rerun 42 to schedule basic regression tests using this pr build and leapp-repository*PR42* as artifacts
  • /rerun-all to schedule all tests (including sst) using this pr build and leapp-repository*master* as artifacts
  • /rerun-all 42 to schedule all tests (including sst) using this pr build and leapp-repository*PR42* as artifacts

Please open ticket in case you experience technical problem with the CI. (RH internal only)

Note: In case there are problems with tests not being triggered automatically on new PR/commit or pending for a long time, please consider rerunning the CI by commenting leapp-ci build (might require several comments). If the problem persists, contact leapp-infra.

@centos-ci
Copy link

Can one of the admins verify this patch?

@leapp-bot
Copy link
Collaborator

This PR has been linked in issue tracker (#OAMG-7319).

@pirat89 pirat89 requested a review from vinzenz August 10, 2022 11:00
@pirat89 pirat89 added this to the 7.9/8.7 milestone Aug 19, 2022
@pirat89 pirat89 added the bug label Aug 19, 2022
Copy link
Member

@vinzenz vinzenz left a comment

Choose a reason for hiding this comment

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

We should ensure

  1. that we always get a str if we handle it that way with the regex.
  2. This is quite testable, so please add some test(s) for it.

leapp/utils/workarounds/fqdn.py Show resolved Hide resolved
@matejmatuska
Copy link
Member Author

/packit build

@vinzenz
Copy link
Member

vinzenz commented Aug 22, 2022

Looks good now, please rebase and squash all the commits into one. Then we can merge it.

Add a wrapper around `socket.getfqdn()` which returns `invalid.hostname`
in cases where hostname, or rather FQDN contains non-ascii and
non-standard characters.

This works around the issue that non-ascii characters can't be inserted
into leapp database.

For the standard for hostnames see `hostname(7)`. The current
implementation doesn't check for length of hostname nor it's individual
segments.
@pirat89 pirat89 dismissed vinzenz’s stale review August 23, 2022 07:27

requested changes has been done

@pirat89 pirat89 merged commit 1cbb4b9 into oamg:master Aug 23, 2022
@pirat89 pirat89 added the changelog-checked The merger/reviewer checked the changelog draft document and updated it when relevant label Aug 23, 2022
pirat89 added a commit to pirat89/leapp that referenced this pull request Aug 23, 2022
## Packaging
-  bumped leapp-framework to 3.1 (oamg#677)

## Framework

### Fixes
- Fixed a problem where passing environment variables to an executed child process modified the environment variables of the parent process (​​oamg#784)
- Ignore invalid FQDNs (oamg#790)

### Enhancements
- Deprecate `reporting.(Tags|Flags)`, replaced by `reporting.Groups` (oamg#677, oamg#781, oamg#788)
- Introduce `is_inhibitor` function (oamg#677)
- Introduce a `Blob` model field (oamg#789)
- Introduce new report JSON schema v1.2.0 (default: 1.1.0) (oamg#677)

## Leapp (tool)

### Fixes
- Handle missing CLI commands gracefully (oamg#785)
- Requires to be executed by root only (oamg#775)
@pirat89 pirat89 mentioned this pull request Aug 23, 2022
pirat89 added a commit to pirat89/leapp that referenced this pull request Aug 23, 2022
## Packaging
-  bumped leapp-framework to 3.1 (oamg#677)

## Framework

### Fixes
- Fixed a problem where passing environment variables to an executed child process modified the environment variables of the parent process (​​oamg#784)
- Ignore invalid FQDNs (oamg#790)

### Enhancements
- Deprecate `reporting.(Tags|Flags)`, replaced by `reporting.Groups` (oamg#677, oamg#781, oamg#788)
- Introduce `is_inhibitor` function (oamg#677)
- Introduce a `Blob` model field (oamg#789)
- Introduce new report JSON schema v1.2.0 (default: 1.1.0) (oamg#677)

## Leapp (tool)

### Fixes
- Handle missing CLI commands gracefully (oamg#785)
- Requires to be executed by root only (oamg#775)

Signed-off-by: Petr Stodulka <pstodulk@redhat.com>
MichalHe pushed a commit that referenced this pull request Aug 23, 2022
## Packaging
-  bumped leapp-framework to 3.1 (#677)

## Framework

### Fixes
- Fixed a problem where passing environment variables to an executed child process modified the environment variables of the parent process (​​#784)
- Ignore invalid FQDNs (#790)

### Enhancements
- Deprecate `reporting.(Tags|Flags)`, replaced by `reporting.Groups` (#677, #781, #788)
- Introduce `is_inhibitor` function (#677)
- Introduce a `Blob` model field (#789)
- Introduce new report JSON schema v1.2.0 (default: 1.1.0) (#677)

## Leapp (tool)

### Fixes
- Handle missing CLI commands gracefully (#785)
- Requires to be executed by root only (#775)

Signed-off-by: Petr Stodulka <pstodulk@redhat.com>

Signed-off-by: Petr Stodulka <pstodulk@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug changelog-checked The merger/reviewer checked the changelog draft document and updated it when relevant
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants