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

Revert PR #6907 as site is not deploying #6981

Merged
merged 1 commit into from
Oct 23, 2023

Conversation

OrlinVasilev
Copy link
Contributor

Thank you for contributing to Velero!

Please add a summary of your change

Does your change fix a particular issue?

Fixes #(issue)

Please indicate you've done the following:

  • Accepted the DCO. Commits without the DCO will delay acceptance.
  • Created a changelog file or added /kind changelog-not-required as a comment on this pull request.
  • Updated the corresponding documentation in site/content/docs/main.

@OrlinVasilev OrlinVasilev added the kind/changelog-not-required PR does not require a user changelog. Often for docs, website, or build changes label Oct 19, 2023
@kaovilai
Copy link
Member

@OrlinVasilev
Copy link
Contributor Author

@kaovilai
Copy link
Member

can you publish your netlify settings?
image

@OrlinVasilev
Copy link
Contributor Author

image

@kaovilai
Copy link
Member

also env
image

@OrlinVasilev
Copy link
Contributor Author

image

@kaovilai
Copy link
Member

Can you unset everything like so?

That way the toml is the source of truth.

Otherwise commit the build settings to site building doc

@OrlinVasilev
Copy link
Contributor Author

The new netlify white theme is so bad :)

@kaovilai
Copy link
Member

I think having it be all in toml would be best for other collaborators to contribute.. if unsetting the config in netlify settings fixes it.

@kaovilai
Copy link
Member

kaovilai commented Oct 19, 2023

So basically what you have in #6981 (comment)
was not settable when I asked in netlify forums. Netlify UI was broken as I noted in https://answers.netlify.com/t/deploy-did-not-succeed-deploy-directory-site-site-public-does-not-exist/103160/3?u=kaovilai

Hence I had to exclusively use the toml to set the right path. Netlify staff verified my toml was good.

@OrlinVasilev
Copy link
Contributor Author

the settings on the Netlify deployment sections are overwritten by the netlify.toml anyway
image
Not sure why Netlify folks stated that it's OKI guess misunderstanding, but it was wrong as we check-out in /opt/build/repo/ and then we build from site and we deploy from site/public.

The current settings are not working for the last 2 weeks

@kaovilai
Copy link
Member

What I have works today.
image

You will note that the package directory is not being overwritten.

@kaovilai
Copy link
Member

kaovilai commented Oct 19, 2023

In any case.. the bug (netlify UI) I reported which prevented this setting from being set was fixed... so we can go ahead with this PR and I will have to set package directory in the netlify UI.

Should add the settings here to the doc tho..

Without the knowledge of velero (netlify ui) build settings here it basically doesn't build prior to #6907

@kaovilai
Copy link
Member

kaovilai commented Oct 19, 2023

Can you at least unset the package directory and trigger a build to try prior to merging?

Package directory
the directory that contains your site files, including the netlify.toml. Set this value only if it is different from the base directory.

netlify.toml is cleary not in site/ as set here

@kaovilai
Copy link
Member

You can trigger by going to https://app.netlify.com/sites/velero/deploys
image

@kaovilai
Copy link
Member

kaovilai commented Oct 19, 2023

You'd have to unset base directory in UI to unset package directory

@OrlinVasilev
Copy link
Contributor Author

@kaovilai
Copy link
Member

woot!

@kaovilai
Copy link
Member

https://app.netlify.com/sites/velero/deploys/6531550e78c95b3eaacf6de1 looks like a deploy for this PR
I didn't mean deploy for this PR.. I mean deploy of current main without this PR

@OrlinVasilev
Copy link
Contributor Author

nope! :(
image
let's merge that :)

@kaovilai
Copy link
Member

kaovilai commented Oct 19, 2023

without setting publish directory to public
Using this build settings for main
#6981 (comment)

I would expect would work without this PR and for future repo forkers who do not have anything set in netlify ui

@OrlinVasilev
Copy link
Contributor Author

once again failed :
image
image

@kaovilai
Copy link
Member

gotcha. Thanks for validating. Must be something else grandfathered in this account on another page.

@OrlinVasilev
Copy link
Contributor Author

with my fix it's ok

@kaovilai
Copy link
Member

kaovilai commented Oct 19, 2023

diffing logs, will let you know if I spot something

@kaovilai
Copy link
Member

❯ Flags
  baseRelDir: true

mine has baseRelDir true

Velero has baseRelDir: false
https://app.netlify.com/sites/velero/deploys/65316342d07d724c54eef0ec#L58

@kaovilai
Copy link
Member

So your PR is not compatible with my netlify flags and vice versa :(

@kaovilai
Copy link
Member

kaovilai commented Oct 19, 2023

I need to ask somebody how to change this flag but based on docs it should be true for everyone else :(

Publish directory: directory that contains the deploy-ready HTML files and assets generated by the build. The directory is relative to the base directory

@kaovilai
Copy link
Member

All paths configured in the netlify.toml should be absolute paths relative to the base directory, which is the root by default (/).

#

@kaovilai
Copy link
Member

kaovilai commented Oct 19, 2023

I'm good with this PR, at least the root cause for why netlify.toml of velero is not compatible with my netlify fork has been identified.

If you don't disagree that new netlify projects from future contributors (which will have baseRelDir: true) should work by default, I will make another PR in the future when I figure out how to change the baseRelDir flag either to document how to for future contributors, or to ask that velero update its netlify config to new default.

Thanks @OrlinVasilev again for troubleshooting with me!

@kaovilai
Copy link
Member

heh.

baseRelDir

Boolean flag intended for backward compatibility. This is set for any sites created since the end of 2019.

When unset:

  • The second pass when searching netlify.toml described above does not apply
  • Paths in netlify.toml are relative to the repository root instead of the build directory

https://github.com/netlify/build/blob/72e33da1f3108c114b88ebd91602df2d325485a2/packages/config/docs/file_paths.md?plain=1#L60-L67

@OrlinVasilev OrlinVasilev requested a review from sseago October 21, 2023 06:49
@OrlinVasilev
Copy link
Contributor Author

@sseago can you review please

@codecov
Copy link

codecov bot commented Oct 22, 2023

Codecov Report

Merging #6981 (36335ab) into main (fd8350f) will decrease coverage by 0.03%.
Report is 2 commits behind head on main.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #6981      +/-   ##
==========================================
- Coverage   61.05%   61.02%   -0.03%     
==========================================
  Files         251      251              
  Lines       26845    26845              
==========================================
- Hits        16389    16381       -8     
- Misses       9303     9310       +7     
- Partials     1153     1154       +1     

see 1 file with indirect coverage changes

@sseago sseago merged commit 107c558 into vmware-tanzu:main Oct 23, 2023
29 of 30 checks passed
@OrlinVasilev
Copy link
Contributor Author

THANK YOU

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/changelog-not-required PR does not require a user changelog. Often for docs, website, or build changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants