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

Remove unnecessary else clause from nsNKeyOutput #4483

Merged
merged 1 commit into from
Jun 23, 2023

Conversation

ckelleyRH
Copy link
Contributor

No description provided.

@ckelleyRH
Copy link
Contributor Author

@edewata @fmarco76 - Note the three new jobs for Packit to do COPR builds for Fedora/CentOS/RHEL.

@ckelleyRH
Copy link
Contributor Author

@edewata @fmarco76 - Note the three new jobs for Packit to do COPR builds for Fedora/CentOS/RHEL.

So the COPR builds for Fedora Rawhide and CentOS 9 work, which is good. The RHEL 9 build fails though, because it is built against RHEL 9.2 and that one has older versions of jss/tomcatjss/ldapjdk. We can probably work around that though by specifying our own COPR repo (the default behaviour is packit makes you a fresh one for each PR). Then we can do the same as we have done here in jss -> tomcatjss/ldapjdk -> pki and then the COPR repo will have the right dependencies in it and it should then work.

What do you think guys?

@fmarco76
Copy link
Member

We can probably work around that though by specifying our own COPR repo

If we specify a COPR repo what is the result of multiple PRs?

pki.spec Outdated
Comment on lines 905 to 914
# Unbundle the FontAwesome fonts
rm %{buildroot}%{_datadir}/pki/common-ui/fonts/fontawesome-webfont.woff
%if 0%{?fedora} > 38
ln -s ../../../fonts/fontawesome4/fontawesome-webfont.woff \
%{buildroot}%{_datadir}/pki/common-ui/fonts/fontawesome-webfont.woff
%else
ln -s ../../../fonts/fontawesome/fontawesome-webfont.woff \
%{buildroot}%{_datadir}/pki/common-ui/fonts/fontawesome-webfont.woff
%endif

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure about removing this code. To my understanding this came from Fedora:

and we do have references to fontawesome-webfont in patternfly.css.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, I see that 56efe4a was breaking the CI, probably because that code only works if we build the theme package (which is not always the case). For reverting a breaking change I think you can just go ahead and merge it, no need to use a PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Apparently it's breaking the CI for PRs too so I went ahead and merge the commit that reverts 56efe4a.

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, it should fast-forward out on merge!

@edewata
Copy link
Contributor

edewata commented Jun 20, 2023

How/where would we specify the COPR repos and which COPR repos will we be using? I just want to make sure that we don't expose private resources to public.

Copy link
Contributor

@edewata edewata left a comment

Choose a reason for hiding this comment

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

LGTM

@ckelleyRH
Copy link
Contributor Author

ckelleyRH commented Jun 21, 2023

We can probably work around that though by specifying our own COPR repo

If we specify a COPR repo what is the result of multiple PRs?

That repo will slowly fill up with builds. We don't use them for anything (for now, at least) so we don't mind that there are multiple builds with weird NVR ordering because of the git hash in there. We just want to see them succeed. What we will probably mind is that there is a 1GB storage limit but the automatic COPR builds here seems to delete the current build and replace it with the new one. That's not quite what we want to happen, but indicates there is some way of controlling how long things live in the repo. Do you know how that works @edewata?

How/where would we specify the COPR repos and which COPR repos will we be using? I just want to make sure that we don't expose private resources to public.

You can specify the Fedora COPR with additional options like this:

  - job: copr_build
    trigger: pull_request
    branch: master
    owner: "@pki"
    project: testing # or whatever we would want to call it
    targets:
      - fedora-development
      - centos-stream-9-x86_64
      - rhel-9-x86_64

There is an option right at the bottom of the repo settings to allow packit to build into that repo. As far as I can tell, we're not exposing anything by doing this.

And also, as mentioned earlier, we can make the RHEL 9 build work by either:

  1. Doing version updates as PRs rather than just pushing them like we do now. Then, providing we respect the build order, we can populate the repo with builds for the new version.
  2. We can bootstrap the repo each time we do a version update by importing SRPMs and doing the first builds manually. Then subsequent builds should all succeed.

@edewata
Copy link
Contributor

edewata commented Jun 21, 2023

Right now the @pki/master repo only provides Fedora builds, but there's an option to create CentOS/RHEL builds. Assuming @pki/master can provide the builds for CentOS/RHEL too, can we configure the repo automatically created by Packit to get the dependencies from @pki/master? This way pki will be able to use the latest (unreleased) jss, tomcatjss, and ldapjdk for those platforms, but the PR builds will remain separate so it wouldn't clutter up @pki/master.

@ckelleyRH
Copy link
Contributor Author

Right now the @pki/master repo only provides Fedora builds, but there's an option to create CentOS/RHEL builds. Assuming @pki/master can provide the builds for CentOS/RHEL too, can we configure the repo automatically created by Packit to get the dependencies from @pki/master? This way pki will be able to use the latest (unreleased) jss, tomcatjss, and ldapjdk for those platforms, but the PR builds will remain separate so it wouldn't clutter up @pki/master.

Good idea, and yes, it is possible to enable additional repos:

  - job: copr_build
    trigger: pull_request
    branch: master
    additional_repos:
      - "https://copr.fedorainfracloud.org/coprs/g/pki/master/"
    targets:
      - fedora-development
      - centos-stream-9-x86_64
      - rhel-9-x86_64

...so we could give the above a go. I'll try it in the morning.

@ckelleyRH ckelleyRH force-pushed the copr-test branch 2 times, most recently from 5f37e06 to a1e6ed3 Compare June 22, 2023 10:09
@ckelleyRH
Copy link
Contributor Author

ckelleyRH commented Jun 22, 2023

I have rebuilt everything in the @pki/master COPR for centos-stream-9-x86_64 and rhel-9-x86_64 - all those builds completed successfully. I have modified .packit.yaml to utilise that repo on PR-generated COPR builds.

I've not quite figured out what the correct format is for adding in the repo, I think I may have to add the repo in for each distro...

@edewata - once I have succeeded, the Fedora job will succeed but the CentOS/RHEL9 9 jobs as well as the Azure pipeline will fail because resteasy-servlet-initializer is unavailable. I guess what we actually want to do is have master run on Rawhide/branched only and have the latest branched branch (v11.4) build against CentOS/RHEL 9. Otherwise, any time there is a breaking change this build will fail until the CentOS/RHEL buildroot catches up "some time" later.

Maybe there is a better way of doing it, thoughts?

@edewata
Copy link
Contributor

edewata commented Jun 22, 2023

Hmm... I actually had built the latest resteasy that provides resteasy-servlet-initializer for F37-F39 in @pki/master. Officially, the resteasy-servlet-initializer will only be shipped in F39 or later, but the @pki/master can be used to provide unofficial dependencies for other platforms. This is necessary since most of us are probably doing development on F38 or F37, and the CI is also running against F38 too. If we enable CentOS/RHEL 9 in @pki/master, we'll need to rebuild resteasy-servlet-initializer for those platforms too. This way when the code in the master branch (PKI 11.5) is finally deployed on CentOS/RHEL 9 later, we'll already know the dependencies/changes that will be required for that to happen.

@ckelleyRH
Copy link
Contributor Author

ckelleyRH commented Jun 22, 2023

Hmm... I actually had built the latest resteasy that provides resteasy-servlet-initializer for F37-F39 in @pki/master. Officially, the resteasy-servlet-initializer will only be shipped in F39 or later, but the @pki/master can be used to provide unofficial dependencies for other platforms. This is necessary since most of us are probably doing development on F38 or F37, and the CI is also running against F38 too. If we enable CentOS/RHEL 9 in @pki/master, we'll need to rebuild resteasy-servlet-initializer for those platforms too. This way when the code in the master branch (PKI 11.5) is finally deployed on CentOS/RHEL 9 later, we'll already know the dependencies/changes that will be required for that to happen.

Did I accidentally delete your build? I have been tinkering with resteasy builds myself, so apologies if I have wiped something - I hope you still have the SRPM? If you do a build in the repo now you get the option for CentOS/RHEL 9 as well as Fedora now if you can easily reinstate it.

@edewata
Copy link
Contributor

edewata commented Jun 22, 2023

OK, I managed to rebuild all packages on Fedora/CentOS/RHEL with some tweaks (e.g. dropping ExclusiveArch) in my COPR repo: https://copr.fedorainfracloud.org/coprs/edewata/pki/builds/
Feel free to use the SRPM from this repo to restore the builds in @pki/master. I'll clean up the changes and merge them to Fedora dist-git. Please note that in my repo I'm still using Jackson 2.14 from F38. If you could use Jackson 2.15 from F39 that would be better. There are also some packages that we don't own, so we would need to work with the maintainers to update them.

@ckelleyRH
Copy link
Contributor Author

OK, I managed to rebuild all packages on Fedora/CentOS/RHEL with some tweaks (e.g. dropping ExclusiveArch) in my COPR repo: https://copr.fedorainfracloud.org/coprs/edewata/pki/builds/ Feel free to use the SRPM from this repo to restore the builds in @pki/master. I'll clean up the changes and merge them to Fedora dist-git. Please note that in my repo I'm still using Jackson 2.14 from F38. If you could use Jackson 2.15 from F39 that would be better. There are also some packages that we don't own, so we would need to work with the maintainers to update them.

I am in the process of rebuilding everything, trying to get "latest Fedora" as the baseline. I am partway through sorting out Jackson (I am doing 2.15) and I have been pushing spec file tweaks as I go.

@ckelleyRH
Copy link
Contributor Author

/packit copr-build

@ckelleyRH
Copy link
Contributor Author

ckelleyRH commented Jun 23, 2023

/packit copr-build

@edewata @fmarco76 - pro tip - you can (re)run packit jobs like this if you have problems. The COPR builds here didn't start, presumably because of some timeout waiting for the CI to start waiting for some other change to pass through. You will need to add your names into .packit.yaml to get it to work.

@sonarcloud
Copy link

sonarcloud bot commented Jun 23, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
3.4% 3.4% Duplication

@ckelleyRH ckelleyRH merged commit 8c2094e into dogtagpki:master Jun 23, 2023
139 checks passed
@ckelleyRH ckelleyRH deleted the copr-test branch June 23, 2023 20:20
@ckelleyRH
Copy link
Contributor Author

Going to merge this now the CI is green again, we can make further improvements in other PRs, thanks @edewata @fmarco76 !

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.

3 participants