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 the needle dir when worker enables cache service #4245

Merged
merged 1 commit into from Oct 8, 2021

Conversation

Amrysliu
Copy link
Contributor

Copy link
Member

@okurz okurz left a comment

Choose a reason for hiding this comment

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

Please put more details in the git commit message details (not just the reference to the ticket). And there is a typo s/diretory/directory/

@Amrysliu Amrysliu changed the title Fix the needle dir when worker has cache diretory Fix the needle dir when worker enables cache service Sep 28, 2021
@Amrysliu
Copy link
Contributor Author

Please put more details in the git commit message details (not just the reference to the ticket). And there is a typo s/diretory/directory/

Thanks. Added more details in commit message.

Copy link
Member

@okurz okurz left a comment

Choose a reason for hiding this comment

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

s/diretory/directory/

I did not understand the sentence "This is because we didn't receive the cache directory when count needle_dir." . What do you mean by "count"? Can you rephrase?

Copy link
Contributor

@Martchus Martchus left a comment

Choose a reason for hiding this comment

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

Right, we should definitely pass through all the parameters. Maybe it makes also sense to just write it as:

sub needledir { productdir(@_) . '/needles' }

And I also don't understand the wording in the commit message. Maybe you don't need to go into that much detail anyways.

@Amrysliu
Copy link
Contributor Author

s/diretory/directory/

I did not understand the sentence "This is because we didn't receive the cache directory when count needle_dir." . What do you mean by "count"? Can you rephrase?

I simplified the commit message. Hope it would be clear. :)

Copy link
Member

@okurz okurz left a comment

Choose a reason for hiding this comment

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

commit message is fine. As you try to fix an actual issue, can you add a test?

There are three parameters in productdir(), but now function needledir()
pass two. The last one `$rootfortests` is used to store the CACHE directory
when worker enables cache service. So we should pass all parameters.

Relate ticket: https://progress.opensuse.org/issues/99378
@Amrysliu
Copy link
Contributor Author

commit message is fine. As you try to fix an actual issue, can you add a test?

commit message is fine. As you try to fix an actual issue, can you add a test?

Added

@Amrysliu Amrysliu merged commit cfe7c5c into os-autoinst:master Oct 8, 2021
@Amrysliu Amrysliu deleted the fix_needles_dir_issue branch October 8, 2021 08:32
@okurz
Copy link
Member

okurz commented Oct 8, 2021

Seems like this wasn't merged automatically due to missing codecov report

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

4 participants