-
Notifications
You must be signed in to change notification settings - Fork 328
add FilesWithNames() to activation #497
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
base: main
Are you sure you want to change the base?
Conversation
kolyshkin
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does not make sense to have two commits here, since the second one merely simplifies the issue you just added in the first one. IOW please squash these.
activation/files.go
Outdated
| @@ -0,0 +1,29 @@ | |||
| // Copyright 2015 CoreOS, Inc. | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The copyright year should be 2025 since this code was written in 2025. Also, IANAL but the copyright holder should probably be Red Hat, Inc. (as Red Hat acquired CoreOS in 2018).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like some other modules use both so I copied that.
activation/files.go
Outdated
| @@ -1,4 +1,5 @@ | |||
| // Copyright 2015 CoreOS, Inc. | |||
| // Copyright 2025 CoreOS, Inc. | |||
| // Copyright 2025 RedHat, Inc. | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's Red Hat
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we match the convention with the rest of the project or correct it here? https://github.com/search?q=repo%3Acoreos%2Fgo-systemd%20redhat&type=code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh well, let's fix that separately. #499
activation/files.go
Outdated
| @@ -1,4 +1,5 @@ | |||
| // Copyright 2015 CoreOS, Inc. | |||
| // Copyright 2025 CoreOS, Inc. | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, there's no "CoreOS, Inc." any longer, that company got acquired by Red Hat in 2018 or so. IANAL but I guess you can omit this line, only leaving Red Hat one.
|
If you can fix the copyright and squash your commits I think this one LGTM |
c3f118d to
f97010a
Compare
reopens #448 which had a broken actions runner.