Skip to content

update to .NET 8.0#77

Merged
christianspecht merged 2 commits into
christianspecht:masterfrom
lessismore-sparkvision:net8.0
Jun 5, 2024
Merged

update to .NET 8.0#77
christianspecht merged 2 commits into
christianspecht:masterfrom
lessismore-sparkvision:net8.0

Conversation

@lessismore-sparkvision

Copy link
Copy Markdown
Contributor

Continued branch net6.0 to net 8.0.
Consider to add mailname to settings.yml

@christianspecht

Copy link
Copy Markdown
Owner

Thanks, looks good!

Two questions:

  1. When I merge this and make a new release, which version number would you give that release?
    On one hand, it's a breaking change - all users who run SCM Backup on an actual machine (and not just via Docker) have to install a new .NET runtime.
    On the other hand, releasing a version 2.0 with no functional changes feels wrong.
    I think I'll just increase the minor version (so the new release would be v1.8.0), but I'm interested in a second opinion.

  2. You wrote:

    Consider to add mailname to settings.yml

    Can you elaborate? It's probably something in the MailkitEmailSender, but it's years since I last touched this file, and from the top of my head I don't know what mailname is. Thanks

@christianspecht

Copy link
Copy Markdown
Owner

Oh, and the Linux build failed. You only changed the AppVeyor file in your PR, but there's also this GitHub Action.

Strange, though. It looks as if the actual build worked (even though the build script installed .NET Core 3.x on the runner), and the tests are failing because the environment variables are missing. Maybe it's because the variables are defined in my repo and the build is running from your fork, not sure.

@lessismore-sparkvision

lessismore-sparkvision commented May 2, 2024

Copy link
Copy Markdown
Contributor Author
  1. Regarding version number - it's in a gray zone for me as well. Following semver.org minor versions should be backward compatible, but this is not. It requires a new runtime. Major versions are for incompatible API changes, but it is not. Personally, I vote for 2.0 with documentation what has changed.
  2. Mailkit has no overload with address only, so used MailboxAddress(string name, string address) with a hardcoded "" for name. My idea was to add name in settings.yml and replace the hardcoded "". (src/ScmBackup/Http/MailKitEmailSender.cs row 25).
  3. I forgot to update github workflow. As you have identified, build script has to be updated to use net8.0 and environmental variables are missing. From the script, it looks like you are using Github secrets to define and set the environmental variables??

@christianspecht

Copy link
Copy Markdown
Owner
  1. Still thinking about the version number.

  2. I'm not against providing a name via config, but is that really important for anybody? I wouldn't mind "SCM Backup" as a hardcoded name ;-)

  3. Yes, the environment variables are coming from Github repository secrets. No idea what to do, so an Action triggered by your fork can read them. I guess we have to wait until I merge it and the action runs in my repo. But the Appveyor build worked, so I don't see why the Actions build wouldn't work.

@lessismore-sparkvision

Copy link
Copy Markdown
Contributor Author

Sorry for the delay. A lof to do privately...

  1. Your choice.
  2. Will use "SCM Backup" as a hardcoded name.
  3. I agree with your guess.

Will update PR with "SCM backup" suggestion tonight.

@christianspecht christianspecht merged commit 1002aff into christianspecht:master Jun 5, 2024
@lessismore-sparkvision lessismore-sparkvision deleted the net8.0 branch June 11, 2024 08:40
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.

2 participants