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

Fix log path #72

Merged
merged 3 commits into from
Aug 1, 2022
Merged

Fix log path #72

merged 3 commits into from
Aug 1, 2022

Conversation

Firstyear
Copy link
Collaborator

Fixes the log path we emit.

@coveralls
Copy link

coveralls commented Mar 4, 2022

Pull Request Test Coverage Report for Build 2758400001

  • 0 of 16 (0.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.07%) to 11.538%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/lib/authserver/ui/new_dir_inst.rb 0 6 0.0%
src/lib/authserver/ui/new_krb_inst.rb 0 10 0.0%
Totals Coverage Status
Change from base Build 2154632754: -0.07%
Covered Lines: 57
Relevant Lines: 494

💛 - Coveralls

Copy link
Member

@lslezak lslezak left a comment

Choose a reason for hiding this comment

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

Note: This changes the translated texts, all translations would be invalidated. Because we are before the Leap 15.4 release and translated texts cannot be changed anymore we will merge it later (after 15.4 is released).

src/lib/authserver/ui/new_dir_inst.rb Outdated Show resolved Hide resolved
@dgdavid
Copy link
Member

dgdavid commented Jul 25, 2022

@Firstyear,

First of all, thank you :)

Although the PR has been out of our radar for a couple of months, I think we can resume it and merge to master now. Are you willing to follow @lslezak's suggestions before continuing?

Regards.

@Firstyear
Copy link
Collaborator Author

Sure, but won't that mean I need to update every translation as well?

@dgdavid
Copy link
Member

dgdavid commented Jul 26, 2022

Sure, but won't that mean I need to update every translation as well?

Yes, please. I mean, every translation touched in this PR. Use the proposed solution for all of them if you don't mind.

Thanks in advance.

@Firstyear
Copy link
Collaborator Author

Sure, but won't that mean I need to update every translation as well?

Yes, please. I mean, every translation touched in this PR. Use the proposed solution for all of them if you don't mind.

Thanks in advance.

I already did?

Copy link
Member

@dgdavid dgdavid left a comment

Choose a reason for hiding this comment

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

@Firstyear,

Sorry, I overlooked some missing format methods in my previous review.

I have added other suggestions too, but just moving the log path out of translations will be enough for having the PR ready to merge.

Please, let me know if you need help with those changes. I'll happily jump in to help you to move this on if needed.

src/lib/authserver/ui/new_dir_inst.rb Outdated Show resolved Hide resolved
src/lib/authserver/ui/new_dir_inst.rb Outdated Show resolved Hide resolved
src/lib/authserver/ui/new_dir_inst.rb Outdated Show resolved Hide resolved
src/lib/authserver/ui/new_krb_inst.rb Show resolved Hide resolved
src/lib/authserver/ui/new_dir_inst.rb Outdated Show resolved Hide resolved
src/lib/authserver/ui/new_krb_inst.rb Show resolved Hide resolved
src/lib/authserver/ui/new_krb_inst.rb Show resolved Hide resolved
src/lib/authserver/ui/new_krb_inst.rb Show resolved Hide resolved
src/lib/authserver/ui/new_krb_inst.rb Show resolved Hide resolved
src/lib/authserver/ui/new_krb_inst.rb Show resolved Hide resolved
@dgdavid
Copy link
Member

dgdavid commented Jul 27, 2022

Sure, but won't that mean I need to update every translation as well?

Yes, please. I mean, every translation touched in this PR. Use the proposed solution for all of them if you don't mind.
Thanks in advance.

I already did?

Not in the src/lib/authserver/ui/new_krb_inst.rb file, as far as I can see.

@dgdavid
Copy link
Member

dgdavid commented Jul 27, 2022

@Firstyear

Forgot all comments about the src/lib/authserver/ui/new_krb_inst.rb file. You are dropping it in #74

Just add those missing formats in the other file and we're done.

Thanks.

@Firstyear
Copy link
Collaborator Author

Just add those missing formats in the other file and we're done.

In what other file? I can't find anything else that needs update ... (I'm not really a ruby programmer at all .... )

@dgdavid
Copy link
Member

dgdavid commented Jul 28, 2022

Just add those missing formats in the other file and we're done.

In what other file? I can't find anything else that needs update ... (I'm not really a ruby programmer at all .... )

I mean, in src/lib/authserver/ui/new_dir_inst.rb. There you followed the @lslezak suggestion, but forgetting about format method. Anyway, I used the Github suggestions for helping with that. Take a look at my previous, unresolved comments. E.g., #72 (comment)

Co-authored-by: David Díaz <1691872+dgdavid@users.noreply.github.com>
@Firstyear
Copy link
Collaborator Author

I mean, in src/lib/authserver/ui/new_dir_inst.rb. There you followed the @lslezak suggestion, but forgetting about format method. Anyway, I used the Github suggestions for helping with that. Take a look at my previous, unresolved comments. E.g., #72 (comment)

Ahh thanks! I didn't know about the _format method. Thanks for the help!

Copy link
Member

@dgdavid dgdavid left a comment

Choose a reason for hiding this comment

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

Since the new_krb_inst.rb file will be removed in a subsequent PR, it LGTM. Thank you.

@dgdavid dgdavid merged commit 43fa3b7 into yast:master Aug 1, 2022
@yast-bot
Copy link

yast-bot commented Aug 1, 2022

✔️ Public Jenkins job #23 successfully finished

@yast-bot
Copy link

yast-bot commented Aug 1, 2022

✔️ Internal Jenkins job #15 successfully finished
✔️ Created IBS submit request #276791

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.

None yet

5 participants