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

Controls: Added metadata to the Controls project to enable publishing to NuGet #16611

Open
wants to merge 43 commits into
base: main
Choose a base branch
from

Conversation

Lamparter
Copy link
Contributor

@Lamparter Lamparter commented Dec 16, 2024

Resolved / Related Issues

Discussed on Discord

Steps used to test these changes

  1. Built the Files controls project
  2. Checked for Roslyn analysers during build
  3. Ensured NuGet package was correctly generated
  4. Inspected NuGet package internals to ensure proper generation and inclusion of XAML resources

WHEN PR IS ACCEPTED (NOT BEFORE)

  • Implement actual secret
  • Set environment to NuGet
  • Remove build on main and PR configurations
  • Add condition for run to only happen on changes made to CurrentVersion.props
  • Replace default GitHub Actions runner token with files-community-bot[bot] token

@Lamparter Lamparter marked this pull request as ready for review December 24, 2024 19:04
@Lamparter Lamparter requested a review from yaira2 December 24, 2024 19:04
@Lamparter
Copy link
Contributor Author

Note for reviewer:
image
The CD fails because there is no such API key provided (please create a NUGET_API_KEY secret in the repository to enable publishing)

@yaira2 yaira2 changed the title Added package metadata to controls project to enable publishing Nuget: Added metadata to the Controls project to enable publishing to Nuget Dec 24, 2024
@Lamparter Lamparter changed the title Nuget: Added metadata to the Controls project to enable publishing to Nuget NuGet: Added metadata to the Controls project to enable publishing to Nuget Dec 24, 2024
@Lamparter Lamparter changed the title NuGet: Added metadata to the Controls project to enable publishing to Nuget NuGet: Added metadata to the Controls project to enable publishing to NuGet Dec 24, 2024
@Lamparter Lamparter requested a review from yaira2 December 24, 2024 20:45
@Lamparter Lamparter marked this pull request as draft January 2, 2025 11:01
@0x5bfa
Copy link
Member

0x5bfa commented Jan 14, 2025

We should do this after v4 or even later. What do you think?

@yaira2
Copy link
Member

yaira2 commented Jan 14, 2025

We can move forward with this as soon as the versioning system is agreed on.

@0x5bfa
Copy link
Member

0x5bfa commented Jan 15, 2025

If you'd like to publish when you publish preview or stable thru Files CD, we can add a parameter text box to input the version. If empty, don't publish; otherwise, try publish with that version string.

@yaira2
Copy link
Member

yaira2 commented Jan 15, 2025

This shouldn't be tied to app releases. We may have a newer control ready for release before an app update, and on the other hand, we may want to release the app without updating the controls. It's best to handle this as a separate action.

@yaira2 yaira2 changed the title NuGet: Added metadata to the Controls project to enable publishing to NuGet Controls: Added metadata to the Controls project to enable publishing to NuGet Feb 9, 2025
@Lamparter Lamparter requested a review from yaira2 February 9, 2025 16:36
@Lamparter
Copy link
Contributor Author

@yaira2 when you approve you'll need to give the bot access to the repo
I think it's the read and write to pull request data permission

Signed-off-by: Lamparter <71598437+Lamparter@users.noreply.github.com>
Signed-off-by: Lamparter <71598437+Lamparter@users.noreply.github.com>
Signed-off-by: Lamparter <71598437+Lamparter@users.noreply.github.com>
Signed-off-by: Lamparter <71598437+Lamparter@users.noreply.github.com>
[skip ci]
[skip ci]
[skip ci]
[skip ci]
[skip ci]
[skip ci]
[skip ci]
Lamparter and others added 5 commits March 18, 2025 19:01
Co-authored-by: Yair <39923744+yaira2@users.noreply.github.com>
Signed-off-by: Lamparter <71598437+Lamparter@users.noreply.github.com>
[skip ci]

Signed-off-by: Lamparter <71598437+Lamparter@users.noreply.github.com>
Co-authored-by: Yair <39923744+yaira2@users.noreply.github.com>
Signed-off-by: Lamparter <71598437+Lamparter@users.noreply.github.com>
Co-authored-by: Yair <39923744+yaira2@users.noreply.github.com>
Signed-off-by: Lamparter <71598437+Lamparter@users.noreply.github.com>
@Lamparter Lamparter requested a review from yaira2 March 18, 2025 19:06

jobs:
build:
runs-on: windows-latest
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
runs-on: windows-latest
runs-on: windows-latest
environment: Deployments

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would strongly recommend against using the same environment for different things.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The app CD workflow and UI controls CD workflow should be in different environments

Copy link
Member

Choose a reason for hiding this comment

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

I grouped them together as they are both deployments. We can separate them if there is a good reason to, but I may have to generate new tokens.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They will need to be separate as the CD for the app requires approval but the CD for the Controls project won't (as it will add extra hassle)

Copy link
Member

@yaira2 yaira2 Mar 18, 2025

Choose a reason for hiding this comment

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

The controls project will require approval as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I still think it makes sense to put them in separate deployment environments as they deploy to separate deployment environments

Co-authored-by: Yair <39923744+yaira2@users.noreply.github.com>
Signed-off-by: Lamparter <71598437+Lamparter@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changes requested Changes are needed for this pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants