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

Tests for kickstart script commands #1302

Merged
merged 1 commit into from
Nov 4, 2024

Conversation

adamkankovsky
Copy link
Contributor

@adamkankovsky adamkankovsky commented Sep 13, 2024

@adamkankovsky adamkankovsky marked this pull request as draft September 13, 2024 08:56
@adamkankovsky adamkankovsky force-pushed the ks-script branch 2 times, most recently from bb8a36d to 96bf92b Compare September 19, 2024 06:58
@adamkankovsky adamkankovsky marked this pull request as ready for review September 19, 2024 07:10
@adamkankovsky adamkankovsky force-pushed the ks-script branch 2 times, most recently from dcc5d33 to 2a2ec60 Compare September 19, 2024 13:16
@adamkankovsky
Copy link
Contributor Author

/test-tmt

@rvykydal
Copy link
Contributor

rvykydal commented Oct 4, 2024

/test-os-variants

Copy link
Contributor

@rvykydal rvykydal left a comment

Choose a reason for hiding this comment

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

This is great work, thank you!
I have some suggestions regarding naming, metadata and reusing:

  • I'd rather make all the tests of a common TESTTYPE, something like ksscript or script so that it is easy to run them together (like in the anaconda PR: /kickstart-test --kstest-pr 1302 --testtype ksscript)
  • I'd name the tests with common prefix, like script-pre instead of pre-script
  • I think we can use fragments (%ksinclude) more, like in ui_text_interactive. This can be done as a follow-up though.

Copy link
Contributor

@rvykydal rvykydal left a comment

Choose a reason for hiding this comment

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

Thanks for using the fragments (like %ksappend common/common_no_payload.ks) but is seems incomplete to me.

script-post.ks.in Outdated Show resolved Hide resolved
script-post.ks.in Outdated Show resolved Hide resolved
script-pre-install.ks.in Outdated Show resolved Hide resolved
@rvykydal
Copy link
Contributor

/test-os-variants

@rvykydal
Copy link
Contributor

/test-tmt

Copy link
Contributor

@rvykydal rvykydal left a comment

Choose a reason for hiding this comment

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

The update looks good to me, however

  • one test is failing on testing farm, not sure why
  • the tests are not run in permian (on /test-os-variants)) because of the .sh files not being executable (
    if not os.access(filepath, os.X_OK):
    , example of a test making executable: 51d6ecb). Sorry for this non-transparent requirement.

@adamkankovsky adamkankovsky force-pushed the ks-script branch 2 times, most recently from 636ddcd to 71700e8 Compare October 22, 2024 13:18
@adamkankovsky
Copy link
Contributor Author

/test-os-variants

@adamkankovsky
Copy link
Contributor Author

/test-tmt

@adamkankovsky
Copy link
Contributor Author

/test-os-variants

@adamkankovsky
Copy link
Contributor Author

/test-tmt

@adamkankovsky
Copy link
Contributor Author

/test-tmt

@adamkankovsky
Copy link
Contributor Author

/test-os-variants

@adamkankovsky
Copy link
Contributor Author

/test-tmt

@adamkankovsky
Copy link
Contributor Author

/test-os-variants

@adamkankovsky
Copy link
Contributor Author

/test-tmt

Copy link
Contributor

@rvykydal rvykydal left a comment

Choose a reason for hiding this comment

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

Looks good to me, great job, thank you.

@KKoukiou KKoukiou merged commit 6ee809c into rhinstaller:master Nov 4, 2024
6 checks passed
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.

3 participants