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

Migrate netcoreapp3.1 and ILogger #214

Merged
merged 10 commits into from
Jun 21, 2021

Conversation

TsuyoshiUshio
Copy link
Collaborator

@TsuyoshiUshio TsuyoshiUshio commented Jun 2, 2021

Currently, the app is written against netcoreapp2.2 that is no longer supported.
@pragnagopa

#202

I start this PR for discussing the design.

The process might be the following, however, I need to learn more to validate it.

  • Update netcoreapp3.1 with removing all the errors
  • Run CI to see if the all test works correctly (How can we run the CI? @sanchitmehta do you know ?)
  • Test with k4apps (By performing zip deploy)
  • Remove unnecessary classes. (e.g. DebugExtensionMiddleWares.cs looks no reference)
  • Enable Console Logging for ASP.NET
  • Remove ITracer and migrate to ILogger
  • Adding more logs for enable debugging on the Lima

@TsuyoshiUshio TsuyoshiUshio marked this pull request as draft June 2, 2021 04:50
@pragnagopa
Copy link
Collaborator

Thanks for the PR! I agree with the approach you described. I recommend sending separate PRs. So this PR can be merged as it is scoped to moving to .Net Core 3.1 @sanchitmehta what do you think?

@TsuyoshiUshio
Copy link
Collaborator Author

I agree with you @pragnagopa !
Hi @sanchitmehta I'm wondering how we can execute this CI against feature/k8se branch ?

@TsuyoshiUshio
Copy link
Collaborator Author

Hi @sanchitmehta

I update the testing, by following rules. However, I sometimes, wondering if my fix is correct. (I don't know the detail usage of Oryx build ) and don't know the context of this test. Could you have a look at my change is correct or not.

Since the Oryx Build is fails a lot. It looks some feature change but not updated the unit tests.
So that I trust the current code is correct. However, some tests are probably not used and probably wrong. I add TODO comments for these tests.

CC @pragnagopa

@TsuyoshiUshio
Copy link
Collaborator Author

Hi @Hazhzeng
Could you help me to review this PR against feature/k8se branch?
The branch has a lot of broken test about the oryx build. I currently, change the test code. Some of them I don't have confidence if it is correct since I don't know about current oryx build usage. (Now I'm trying to learn the usage) However, it might be good for reviewed by someone who is an expert about it.

@TsuyoshiUshio TsuyoshiUshio marked this pull request as ready for review June 16, 2021 23:07
@TsuyoshiUshio
Copy link
Collaborator Author

Hi @pragnagopa @sanchitmehta

I tested with this change deploy with zip deploy and container app it works.
I'm wondering if we need to test other use cases like WebApp or good to go? If it is ok, Let get merged! I'll update the PR of the k4apps side.

@TsuyoshiUshio
Copy link
Collaborator Author

The next PR will be adding more logs and switch it into ILogger.

@sanchitmehta
Copy link
Collaborator

Needs a fix for Local Git Deploys as well:

public static string KUDUCOMMAND = "benv dotnet=2.2 dotnet \"$" + EXEPATH + "\" " +
. This is the post recieve hook that we create for Local Git. It needs to refer the self-contained binary

@TsuyoshiUshio
Copy link
Collaborator Author

Hi @sanchitmehta
That is a very good catch! I also find other places that requires to change (probably not used) also change the unit testing to fit it.

I also build the image with this version and tested with lima arc cluster. It works.

@pragnagopa pragnagopa merged commit 45644b1 into Azure-App-Service:feature/k8se Jun 21, 2021
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