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.

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

add micro-based httpd image for c8s, c9s and Fedora #138

Merged
merged 14 commits into from
Oct 25, 2022
Merged

add micro-based httpd image for c8s, c9s and Fedora #138

merged 14 commits into from
Oct 25, 2022

Conversation

pkubatrh
Copy link
Member

No description provided.

@pkubatrh
Copy link
Member Author

[test]

1 similar comment
@pkubatrh
Copy link
Member Author

[test]

@pkubatrh pkubatrh changed the title add ubi8-micro-based httpd image [WIP] add micro-based httpd image for rhel8 and rhel9 Jun 22, 2022
@pkubatrh pkubatrh force-pushed the micro branch 2 times, most recently from c12f6f2 to 3a9d09c Compare June 27, 2022 07:50
@pkubatrh
Copy link
Member Author

[test]

@pkubatrh
Copy link
Member Author

[test-all]

@pkubatrh pkubatrh mentioned this pull request Aug 10, 2022
Merged
@pkubatrh pkubatrh force-pushed the micro branch 2 times, most recently from 0e3fcf6 to f6b93a4 Compare August 10, 2022 09:08
@pkubatrh
Copy link
Member Author

[test-all]

@pkubatrh
Copy link
Member Author

[test-all]

@pkubatrh pkubatrh changed the title [WIP] add micro-based httpd image for rhel8 and rhel9 [WIP] add micro-based httpd image for rhel8, c9s and Fedora Aug 10, 2022
@pkubatrh pkubatrh mentioned this pull request Aug 10, 2022
Merged
@pkubatrh
Copy link
Member Author

[test-all]

2 similar comments
@pkubatrh
Copy link
Member Author

[test-all]

@pkubatrh
Copy link
Member Author

[test-all]

@pkubatrh pkubatrh changed the title [WIP] add micro-based httpd image for rhel8, c9s and Fedora add micro-based httpd image for rhel8, c9s and Fedora Aug 22, 2022
@pkubatrh
Copy link
Member Author

[test-all]

@pkubatrh
Copy link
Member Author

[test-all]

3 similar comments
@pkubatrh
Copy link
Member Author

[test-all]

@pkubatrh
Copy link
Member Author

[test-all]

@pkubatrh
Copy link
Member Author

[test-all]

hhorak
hhorak reviewed Oct 11, 2022
View reviewed changes
2.4-micro/2.4-micro
@@ -0,0 +1 @@
.
Copy link
Member

Choose a reason for hiding this comment

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

@phracek said in case of nginx-container that this kind of symlink breaks upstream->downstream. Not sure whether also in this case.

Copy link
Member Author

@pkubatrh pkubatrh Oct 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

What is the issue with this approach? Iirc cwt during syncs actually creates this symlink by default.

@pkubatrh
Copy link
Member Author

[test-all]

1 similar comment
@pkubatrh
Copy link
Member Author

[test-all]

@pkubatrh
Copy link
Member Author

pkubatrh commented Oct 19, 2022 •
edited
Loading

De-duplication should be possible as I do not think I made that many changes from full 2.4, but will need some work. We can follow up in a separate PR.

@pkubatrh
Copy link
Member Author

Centos7 CI failing due to unavailable "dnf-utils" which is an issue of the underlying infrastructure (there is no dnf in centos7).

@phracek Can you take a look again if everything is ok and as expected with this PR now?

@pkubatrh
Copy link
Member Author

[test-all]

phracek
phracek requested changes Oct 20, 2022
View reviewed changes
Copy link
Member

@phracek phracek 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 bringing httpd-micro container. Can you please look at my proposals and address them or comment on them? Thanks.

2.4-micro/2.4-micro Show resolved Hide resolved
2.4-micro/Dockerfile.fedora Show resolved Hide resolved
2.4-micro/root/usr/share/container-scripts/httpd/README.md Outdated Show resolved Hide resolved
2.4-micro/root/usr/share/container-scripts/httpd/README.md Show resolved Hide resolved
2.4-micro/s2i/bin/assemble
@@ -0,0 +1,21 @@
#!/bin/bash
Copy link
Member

Choose a reason for hiding this comment

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

Please create a symlink as @hhorak proposes. It makes sense to me as well.

@pkubatrh
Copy link
Member Author

pkubatrh commented Oct 21, 2022 •
edited
Loading

After committing the suggestion I for some reason cannot reply on the rest of the comments :/ So adding the responses here:

What does it mean micro? It is not mentioned here. What is the difference between 2.4 and 2.4-micro. This could be misleading.

Right I forgot about this. I will add some part that explains the differences.

Please create a symlink as @hhorak proposes. It makes sense to me as well.

As mentioned in a previous comment, I am planning to move to shared files in a separate PR, as I do not want to hold this up much longer.

@pkubatrh
Copy link
Member Author

[test-all]

@pkubatrh @phracek
Update 2.4-micro/root/usr/share/container-scripts/httpd/README.md
9244323 
Co-authored-by: Petr Hracek <phracek@redhat.com>
@pkubatrh
Copy link
Member Author

[test-all]

phracek
phracek approved these changes Oct 25, 2022
View reviewed changes
Copy link
Member

@phracek phracek left a comment

Choose a reason for hiding this comment

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

LGTM. Tests are passing. Let's get merge it and solve issues like common scripts by separate PR. Please create an issue for it, so we do not forget.

@pkubatrh pkubatrh mentioned this pull request Oct 25, 2022
Open
@pkubatrh pkubatrh merged commit 756ec68 into sclorg:master Oct 25, 2022
@pkubatrh pkubatrh deleted the micro branch October 25, 2022 08:09
@pkubatrh pkubatrh mentioned this pull request Nov 10, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hhorak hhorak hhorak left review comments

@phracek phracek phracek approved these changes

Assignees
No one assigned
Labels
ready for review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@pkubatrh @hhorak @phracek