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

docker: add action to deploy windows build image #3780

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

gdams
Copy link
Member

@gdams gdams commented Oct 15, 2024

step 1 of #3217

Checklist
  • commit message has one of the standard prefixes
  • faq.md updated if appropriate
  • other documentation is changed or added (if applicable)
  • playbook changes run through VPC or QPC (if you have access)
  • VPC/QPC not applicable for this PR
  • for inventory.yml changes, bastillion/nagios/jenkins updated accordingly

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

A block has been put on this Pull Request as this repository is temporarily under a code freeze due to an ongoing release cycle.

If this pull request needs to be merged during the release cycle then please comment /merge and a PMC member will be able to remove the block.

If the code freeze is over you can remove this block by commenting /thaw.

Copy link
Member

@sxa sxa left a comment

Choose a reason for hiding this comment

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

@gdams
Copy link
Member Author

gdams commented Oct 15, 2024

Can you add in a doc change to the table in https://github.com/adoptium/infrastructure/blob/master/FAQ.md#what-about-the-builds-that-use-the-dockerbuild-tag too please as part of this PR?

done

@github-actions github-actions bot added the doc label Oct 15, 2024
@gdams gdams requested a review from sxa October 15, 2024 14:54
Copy link
Member

@sxa sxa left a comment

Choose a reason for hiding this comment

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

Approving in principle for when we've got this tested (We probably don't want to be running of my infra branch so we should perhaps merge those changes - or equivalents - into this PR once verified as reliable.

Just as a point of clarification - the use of this won't be going near production until after the release cycle so if this is a bit rough round the edges (quite likely) it will still be nice to have the image creation/publish process in place.

Copy link
Member

@sxa sxa left a comment

Choose a reason for hiding this comment

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

I'm going to block this for now as the changes to remove openj9 support (added after I gave my approval) should not be in here. If we want to skip the inclusion of those installs in the adoptium build containers then let's skip the roles in the dockerfile for now but since our build scripts do support non-temurin builds and other projects may be using it this should be a separate issue for discussion and not just done inside this PR.

@gdams
Copy link
Member Author

gdams commented Oct 16, 2024

I'm going to block this for now as the changes to remove openj9 support (added after I gave my approval) should not be in here. If we want to skip the inclusion of those installs in the adoptium build containers then let's skip the roles in the dockerfile for now but since our build scripts do support non-temurin builds and other projects may be using it this should be a separate issue for discussion and not just done inside this PR.

updated PTAL

@gdams
Copy link
Member Author

gdams commented Oct 16, 2024

/merge - this is unrelated to the release and it would be good to get the docker image published in Azure so we can begin phase 2

Copy link

Approval to merge during the lockdown cycle

Please can two Adoptium PMC members comment /approve?

@karianna
Copy link
Contributor

/approve

force: no
checksum: 22bae6c3ddf1a464b285784599eef8698f64dde24378c77e42522a536b88cbbc
checksum_algorithm: sha256
win_shell: c:\cygwin64\bin\curl -L -o /cygdrive/c/temp/ant-contrib.zip https://sourceforge.net/projects/ant-contrib/files/ant-contrib/ant-contrib-1.0b2/ant-contrib-1.0b2-bin.zip
Copy link
Member

Choose a reason for hiding this comment

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

We need to add in something here to verify the download on this download in the absence of the checksum check that was in win_get_url

Copy link
Member Author

Choose a reason for hiding this comment

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

I couldn't consistently get the same shasum on that download URL (I have no idea why and not sure if we should be worried about this)

Copy link
Contributor

Choose a reason for hiding this comment

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

I would be concerned.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed @karianna ... We had a SHA check with the previous win_get_url operation so if something is preventing that when using curl then that would definitely need to be understood before this can go in.

Copy link
Member

Choose a reason for hiding this comment

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

(Since I'm going to be away for most of the next week, if these issues are resolved then feel free to dismiss my review)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants