Skip to content

[ELY-2547] Add Elytron Tool option to overwrite CLI script #2194

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 1 commit into
base: 2.x
Choose a base branch
from
Open

[ELY-2547] Add Elytron Tool option to overwrite CLI script #2194

wants to merge 1 commit into from

Conversation

lvydra
Copy link
Contributor

@lvydra lvydra commented Sep 18, 2024

@lvydra lvydra mentioned this pull request Sep 18, 2024
Closed
@lvydra
Copy link
Contributor Author

lvydra commented Sep 18, 2024

@darranl Well, it seems that FileSystemEncryptRealmCommandTes.testOverwritingScriptFileTrue is intermittently failing. I will try to tweak it.

@darranl darranl added the +1 DAL label Sep 19, 2024
@fjuma
Copy link
Contributor

fjuma commented Sep 19, 2024

@lvydra Looks like this is still flaky on CI.

@fjuma fjuma added the hold label Sep 19, 2024
@lvydra
Copy link
Contributor Author

lvydra commented Sep 20, 2024

@fjuma @darranl Please rerun the tests to confirm if the changes are stable.

@darranl
Copy link
Contributor

darranl commented Sep 20, 2024

Last run was green, running again for stability.

@fjuma
Copy link
Contributor

fjuma commented Sep 20, 2024

Last run was green, running again.

@fjuma
Copy link
Contributor

fjuma commented Sep 20, 2024

@lvydra Just so we can reference this later on if needed, would you be able to add a quick comment about the intermittent failure and how it was fixed?

@lvydra
Copy link
Contributor Author

lvydra commented Sep 23, 2024 •
edited
Loading

Sure @fjuma . In FileSystemEncryptRealmCommandTest I have been using File.lastModified() method to check if the file has been modified, but as I have found out later this method does not always return the latest value on Windows probably due to the file attributes caching.
So I switched to assertion based on file content.

@darranl darranl removed the hold label Sep 24, 2024
@lvydra
Copy link
Contributor Author

lvydra commented Nov 15, 2024

Hi @fjuma, could you look at this one again, thanks.

rhusar
rhusar suggested changes Mar 6, 2025
View reviewed changes
.../src/test/resources/filesystem-integrity/fs-unsigned-realms/fsRealmAl/a/l/alice-MFWGSY3F.xml Outdated
<attribute name="role" value="admin"></attribute>
</attributes>

</identity>
Copy link

Choose a reason for hiding this comment

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

You should run a formatter on this XML file ;-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @rhusar, updated.

@lvydra lvydra requested a review from rhusar March 10, 2025 07:13
rhusar
rhusar approved these changes Mar 12, 2025
View reviewed changes
Copy link

@rhusar rhusar left a comment

Choose a reason for hiding this comment

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

👍🏼 No more comments from me.

@ivassile
Copy link
Contributor

ivassile commented Apr 2, 2025 •
edited
Loading

@fjuma / @darranl This PR is ready and can be merged.

@darranl darranl mentioned this pull request Apr 13, 2025
Open
@darranl
Copy link
Contributor

darranl commented Apr 13, 2025

@lvydra sorry but we still seem to have CI failures #2272

darranl
darranl requested changes Apr 13, 2025
View reviewed changes
Copy link
Contributor

@darranl darranl left a comment

Choose a reason for hiding this comment

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

See comments

@lvydra
Copy link
Contributor Author

lvydra commented Apr 14, 2025

Hi @darranl, thanks for looking at it. This change should help.

@Skyllarr
Copy link
Contributor

@lvydra sorry but we still seem to have CI failures #2272

@darranl You requested changes on this PR because the CI was failing (#2272). This PR was updated and the CI now passes here. So you can merge this one and close #2272

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@darranl darranl darranl requested changes

@fjuma fjuma fjuma approved these changes

@Skyllarr Skyllarr Skyllarr approved these changes

+1 more reviewer

@rhusar rhusar rhusar approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
+1 DAL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@lvydra @fjuma @darranl @ivassile @Skyllarr @rhusar